Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with not highlighted snippets and automatically scroll to snippets #168

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

FrostyX
Copy link
Member

@FrostyX FrostyX commented Sep 7, 2024

Two unrelated changes in this PR:

@FrostyX
Copy link
Member Author

FrostyX commented Sep 7, 2024

Okay, this seems to work great for about half of the snippets. For the other half, it either scrolls a little too much or not enough.

@xsuchy
Copy link
Member

xsuchy commented Sep 9, 2024

Hmm, when I try it in docker-compose, it tells me I have no data. :(
But just by reading the code it LGTM.

@FrostyX
Copy link
Member Author

FrostyX commented Sep 9, 2024

Do not merge yet though. There must be some bug in the calculation of how many lines should be scrolled. If we want ship the fix for not highlighted snippets, I'd rather split it into two PRs.

Copy link
Collaborator

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried about 7 different reviews and in most of them it worked so well!

Comment on lines 159 to 164
;; I experimentally figured out this number but the the applicable CSS
;; line-height is `--bs-body-line-height' which is 1.5 and the
;; font-size is 0.875em which equals to 14px. The `14 * 1.5' number
;; would scroll good enough but when adding 0.3, it positions the
;; snippet more close to the center.
lineheight 21.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reason why webdev is so hard :D

Fix fedora-copr#166

The `partition` function takes only doesn't include partitions with
fewer items than `n`. Therefore when we have an odd number of
snippets, the last one was not being highlighted.
@FrostyX
Copy link
Member Author

FrostyX commented Sep 15, 2024

Okay, this seems to work great for about half of the snippets. For the other half, it either scrolls a little too much or not enough.

I found and fixed the issue, so scrolling to snippets now works in 99% of cases. There are still two special cases that don't work:

  • We only scroll vertically, horizontal scroll isn't implemented. So if a snippet is only a few words, not the whole line, and is so far to the right that we need to scroll horizontally, user needs to do the horizontal scroll manually
  • When a snippet is not in the first file, we automatically switch tab to the correct file and scroll. But when user clicks on the snippet in the right column and the file needs to be switched, we switch it but we don't automatically scroll

I'd prefer merging now and fixing these corner cases in a followup PR

When the review page is loaded, automatically scroll down to the
displayed snippet. Also, when user clicks on a snippet in the right
column, automatically scroll to it as well.
@FrostyX
Copy link
Member Author

FrostyX commented Sep 15, 2024

I don't understand why the pre-commit fails because it doesn't show any error:

linting took 91ms, errors: 0, warnings: 0

Lint Clojure.............................................................Passed
Error: Process completed with exit code 1.

@nikromen nikromen merged commit c9de8a6 into fedora-copr:main Sep 16, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants