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 TOC highlighting #234

Merged
merged 4 commits into from
Aug 18, 2021
Merged

Fix TOC highlighting #234

merged 4 commits into from
Aug 18, 2021

Conversation

codemonkey800
Copy link
Collaborator

@codemonkey800 codemonkey800 commented Aug 13, 2021

Description

Closes #157

This PR fixes the active header highlighting for the TableOfContents component. This updates the logic so that it implements the following missing behavior:

when I click the second-to-last item in the nav:❗️it does not apply selected style to the clicked-nav item (instead keeps same style on last section's nav item). The treatment should change to show second-to-last section's nav item in selected treatment. (Also the page doens't scroll, but that's to be expected because the second-to-last section is also too short to touch the top... but if it's not too short then it should).

Demos

https://fix-toc-frontend.dev.imaging.cziscience.com/plugins/napari-bioformats

Highlight Selected

Clicking on a TOC will highlight the selected item, even if there's not enough space for it to be active on scroll, and it'll stay highlighted until the user scrolls again.

highlight-selected.mp4

Highlight on Initial Load

When sharing a link to a heading directly, the page will:

  1. Scroll to the heading
  2. Highlight the corresponding TOC item
highlight-selected-on-load.mp4

Highlight on Reload

Chrome / Edge / Safari

The above browsers will maintain the previous scroll position on reload and highlight the next header in the viewport.

highlight-selected-on-reload.mp4

Firefox

Firefox is the only browser that scrolls back to the heading on reload (instead of maintaining the previous scroll position).

highlight-selected-on-reload-firefox.mp4

Copy link
Member

@kne42 kne42 left a comment

Choose a reason for hiding this comment

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

nice job, LGTM!

@neuromusic
Copy link
Collaborator

neuromusic commented Aug 16, 2021

nice!! super clean & consistent

Copy link
Collaborator

@neuromusic neuromusic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@klai95 klai95 left a comment

Choose a reason for hiding this comment

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

Nicely done! LGTM!

Copy link

@liaprins-czi liaprins-czi left a comment

Choose a reason for hiding this comment

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

Thanks for all your efforts on this issue! I actually think that originally (before this PR) it was working almost as I expected it to, but not the changes in the PR are making it do unexpected things from my perspective.

In the first video it shows the highlightign applying to the last nav item when scroll to the bottom, but I would not expect that to ever happen (unless user clicked that nav item). In the following videos, it shows the highlight applying to nav items that after scrolling away from them and then back down, but they are too low down for the highlighting to make sense when scrolling (only when initially clicked).

Then again, everyone else is not seeming confused by this LOL, and maybe it is too tricky to get the UX I am describing to be worth the time and effort; if so no worries! And let me know if you want to hop on a call to discuss if this is not clear!

@codemonkey800
Copy link
Collaborator Author

After discussing with @liaprins-czi , the incorrect behavior was that the TOC was jumping to the last section when scrolling to the bottom.

I've removed the logic for jumping to the last section and updated the Highlight Selected demo:

highlight-selected.mp4

Copy link

@liaprins-czi liaprins-czi left a comment

Choose a reason for hiding this comment

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

💙 it! This looks / functions perfectly to me! Thanks for all your efforts!

@codemonkey800 codemonkey800 merged commit 1fe9ce7 into main Aug 18, 2021
@codemonkey800 codemonkey800 deleted the jeremy/toc-highlighting branch August 18, 2021 17:33
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.

right nav highlighting jumps to end
5 participants