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 for clicking UX for last few headings when they cannot be scrolled to #370

Merged
merged 6 commits into from
Jan 31, 2025

Conversation

tscanlin
Copy link
Owner

@tscanlin tscanlin commented Jan 3, 2025

fix for clicking UX for last few headings when they cannot be scrolled to. This is hoping to address this comment from storybook:
storybookjs/storybook#26470 (comment)

@Sidnioulz
Copy link

Sidnioulz commented Jan 6, 2025

Hey Tim, thanks for taking the time to build this! ❤️

I tested your branch and it's overall a great improvement! This will definitely satisfy most users.

The only issue I see is if I navigate up from the bottom anchor (http://localhost:3001/tocbot#license), as soon as it's off the screen, I get sent straight to 'Troubleshooting' as per the original algorithm (the first heading to be above the top of the viewport), but I'd expect to go to 'Contributing / Steps to publish' instead. That's why I was suggesting finding the threshold element to detect 'bottom mode' in my original comment, and using that element as a trigger for either mode.

I'm curious how const isBottomMode = activeHeading.offsetTop > scrollEl.offsetHeight - (scrollEl.clientHeight * 1.4) - options.bottomModeThreshold is obtained. As a user, I would struggle to understand what use to make of bottomModeThreshold because the documentation doesn't explain how it's used and what unit to pass, and looking at the source code I am still confused. Am I supposed to tweak this value based on my content length?

If you're able to make a NPM pre-release, we could test this in situation in Storybook to see how it behaves with different content lengths.

@tscanlin
Copy link
Owner Author

tscanlin commented Jan 8, 2025

Hey, thanks for the feedback, and I'm glad you think this is an improvement. I appreciate you working with me on this and I think that our efforts will help to ensure that users have a good experience that makes sense.

I'd like to understand better your idea of "finding the threshold element to detect 'bottom mode'", my understanding was that I could just find any headings that are at a y-offset greater than total scroll height available - scroll viewport height and those would essentially be unable to scroll to and would trigger bottomMode.

As far as the comment about jumping back up to "Troubleshooting" instead of gradually triggering other headings that cannot be scrolled to on the way back up that seems like it could be a bit tricky and doesn't seem intuitive to me. The fix in this PR is mainly meant to fix the issue for if you click a link and it is not able to be scrolled to then it should make that link show as highlighted in the right. bottomModeThreshold is meant for how many px from the bottom of the page that behavior should last for. After you scroll past 20-30 px it reverts to the previous behavior and highlights as normal (default should be fine for most folks). This is not meant to make all the links something you can scroll to or otherwise highlighted via scroll if they are out of the range. I'm not sure how that could really be done in an intuitive way but if you have ideas for that logic I'm open to it.

I'm happy to make a pre release though to help with your testing. Let me do that and once you have a chance to test it more we can figure out if any other changes are needed here. Thanks!

@tscanlin
Copy link
Owner Author

tscanlin commented Jan 8, 2025

You can try out this pre-release version, let me know how it works for you:
tocbot@4.32.3-0

Thanks!

@Sidnioulz
Copy link

I'd like to understand better your idea of "finding the threshold element to detect 'bottom mode'", my understanding was that I could just find any headings that are at a y-offset greater than total scroll height available - scroll viewport height and those would essentially be unable to scroll to and would trigger bottomMode.

Agree! These should all be concerned by the bottomMode logic.

As far as the comment about jumping back up to "Troubleshooting" instead of gradually triggering other headings that cannot be scrolled to on the way back up that seems like it could be a bit tricky and doesn't seem intuitive to me.

If you have multiple headers that can trigger bottom mode, you can display whichever one is visible and closest to the bottom of the screen. So in your repo's example, once License isn't visible any more, Contribution is the "bottom mode" header that's closest to the bottom and visible.

This will make the behaviour of the tocbot highlight more organic instead of leaving gaps when scrolling up and down.

The fix in this PR is mainly meant to fix the issue for if you click a link and it is not able to be scrolled to then it should make that link show as highlighted in the right. bottomModeThreshold is meant for how many px from the bottom of the page that behavior should last for. After you scroll past 20-30 px it reverts to the previous behavior and highlights as normal (default should be fine for most folks).

I feel like you've paid most of the price (in terms of software maintenance) for fixing all highlighting bugs by introducing headers that highlight differently from the existing algorithm, but you're not fully reaping the benefits by trying to keep the bugfix as small as possible.

There are now two logics for headers to be highlighted, which introduces a bunch of user behaviour edge cases. By addressing the edge cases, you can not only fix the initial bug report but prevent new ones, since there are still behaviours that feel inconsistent. And much of the price is already paid, the edge cases revolve around scrolling within the new logic.

I'm happy to make a pre release though to help with your testing. Let me do that and once you have a chance to test it more we can figure out if any other changes are needed here. Thanks!

I'll be a bit busy in the coming weeks, will likely not test integration in Storybook immediately, sadly.

@tscanlin
Copy link
Owner Author

"If you have multiple headers that can trigger bottom mode, you can display whichever one is visible and closest to the bottom of the screen. So in your repo's example, once License isn't visible any more, Contribution is the "bottom mode" header that's closest to the bottom and visible. This will make the behaviour of the tocbot highlight more organic instead of leaving gaps when scrolling up and down."

In this case though what you are proposing seems like it could maybe skip over sections like the "Contributing" section especially if it didn't have that much content below it. I feel like adding the highlighting for sections that are clicked on in bottom mode makes sense, but trying to accommodate scrolling up without jumping seems like it could cause more complexity and may lead to some other headings getting skipped. For example when do you switch back then to the normal way?

"There are now two logics for headers to be highlighted, which introduces a bunch of user behaviour edge cases. By addressing the edge cases, you can not only fix the initial bug report but prevent new ones, since there are still behaviours that feel inconsistent."

I'd like to better understand what feels inconsistent to you here. Aside from the jumping back up after scrolling past the threshold to resume the same heading highlighting logic as before I'd argue that the changes here are fairly minimal and even that feels pretty consistent since its resuming the same logic as before.

Maybe we can proceed with this change if you agree its a step in the right direction and we can revisit later if there are other improvements you'd like to see and you have more time to work on this with me?

@Sidnioulz
Copy link

I'd like to better understand what feels inconsistent to you here.

As you scroll back up after clicking, some sections are never highlighted as the current section, even if they're fully on screen whereas the highlighted section's heading may be far above.

Say you click to this bottom section with tocbot:
image

Then you scroll up just a little bit:
image

The middle section is 100% visible on screen. The top section's heading is far, far above and just a few % of the section are visible. Yet, tocbot says you are in the top section.

This is what feels inconsistent to me, and why I would prefer for this specific event to lead to a different outcome.

For example when do you switch back then to the normal way?

That is why I had imagined the need to find the threshold element, the one from where you know you can just fetch the first header above the top of the viewport.

Now, looking back on this and having forgotten much of the specifics of either approach, I feel there's a choice to be made between switching sections when the heading above the viewport changes vs. when the "current" section's heading leaves the bottom of the viewport.

Maybe we can proceed with this change if you agree its a step in the right direction and we can revisit later if there are other improvements you'd like to see and you have more time to work on this with me?

I do think it's an improvement. I suspect it will not entirely eliminate the perception of bugginess for Storybook users, as I noticed the above scenario immediately when testing, but it is indeed markedly better and I agree with you it's worth putting out there!

@tscanlin
Copy link
Owner Author

Ah yeah, that makes sense and I understand your concern. To describe how tocbot currently works, if the top of the viewport touches top of a heading, then that heading will stay highlighted as long as the top of the viewport scroll area is anywhere between the top of the current header element and the bottom of the content for that header (all the elements before the next header below it). So I agree it can seem strange that tocbot shows a heading as highlighted when it is no longer visible, but it only does that as long as the top of the scrolling view port is still within the content for that heading.

You could change instead to go for whichever header element is currently visible (and top / above any others) as an alternative but then if you were scrolled lets say 50px past the "Top section" header in your example and could see the "Middle section" header but were still essentially barely scrolled into the content at the top of the top section. It would seem to me that the logic should still indicate you are in the top section, not the middle section, right?

I'd say a solution for this should also consider when there are more headings close together (vs spread out) as in the case of tocbot's homepage and many of the autogenerated docs pages I've seen tocbot being used in storybook as well. For example if you have multiple headings near the bottom that can't be scrolled to I think it makes it difficult to accommodate any sensible scroll action for them without potentially skipping other headings that could be short on content near the bottom of the page or just above them.

Thanks again for your input and helping make the UX for this better, much appreciated!!

https://tscanlin.github.io/tocbot/#contributing
image

@tscanlin tscanlin merged commit 129466d into master Jan 31, 2025
4 checks passed
@tscanlin tscanlin deleted the fix-clicking-last-few-items branch January 31, 2025 06:01
@tscanlin
Copy link
Owner Author

This change is published in tocbot@4.33.0

@tuyuritio tuyuritio mentioned this pull request Feb 5, 2025
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.

2 participants