-
-
Notifications
You must be signed in to change notification settings - Fork 116
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
Conversation
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 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. |
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. 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! |
You can try out this pre-release version, let me know how it works for you: Thanks! |
Agree! These should all be concerned by the bottomMode logic.
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.
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'll be a bit busy in the coming weeks, will likely not test integration in Storybook immediately, sadly. |
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?
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? |
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!! |
This change is published in |
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)