-
Notifications
You must be signed in to change notification settings - Fork 328
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
[WIP] [ENH] Implement new scrollspy #2119
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I haven't tested it yet, so I may have some more feedback but I thought you might appreciate me sharing my code feedback before waiting on me to test it.
root: null, // Target the viewport | ||
// Offset the rootMargin slightly so that intersections trigger _before_ headings begin to | ||
// go offscreen | ||
rootMargin: `${-2 * document.querySelector("header.bd-header").getBoundingClientRect().bottom}px 0px 0px 0px`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the bottom coordinate multiplied by -2? could you explain that in a code comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a comment in code, but...
You need to pad in the bottom coordinate by at least the -1*<height of the top menu bar>, because that sticky top bar obscures the top of the screen - which is where the IntersectionObserver would otherwise be triggered.
In testing, I found that if I clicked on a link in the TOC, the main article scrolls to the associated heading. However the intersection observer doesn't trigger for the article heading. I think this is because clicking on the TOC link places the top of the heading bounding box at the top of the viewport, so there's no overlap between the root and the observed heading. So annoyingly, you'd click on a link and it wouldn't get highlighted in the TOC, even though the viewport would appear to be on the correct heading.
I tried setting rootMargin: -(1 + <top nav bar height>)
to offset it by 1 pixel inward but this doesn't seem to work super well. Maybe it has something to do with whether the IntersectionObserver triggers on the bounding boxes/border overlap of the observed element? If you have any insight about what the right solution is here I'm eager to hear it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll poke at it. Setting the top root margin to double the header seems wrong.
074b873
to
6370851
Compare
I just now rebased to see if we can get a working preview build. |
Awesome, I'll get these comments addressed now 🚀 |
Okay, I gave this a whirl on the read the docs preview build, and for the most part it's great! There's just one thing. I know you're already aware of this, but I find it really annoying that I can sometimes click on a link in the right sidebar table of contents and sometimes (when the corresponding heading is near bottom of the page and too close to headings above) it doesn't highlight that link in the TOC after I click it. This doesn't match the pattern established in the rest of the site, where clicking a TOC link (left bar or header nav) highlights that link. This feels like a big enough pattern break and UX frustration to be a blocker. I think without this being fixed I would prefer merging my PR over this one, even if it removes scroll spying, simply because it gets the TOC link highlighting correct. What do you think? Is this something you want to fix? Would you like me to look into fixing it? |
8e3841c
to
38d46a0
Compare
Agreed, but what's the right way to handle this? I think with the current machinery, as the page scrolls down to the selected heading it will highlight the TOC elements you see as the viewport scrolls down. I can think of a few ways of doing this with |
Co-authored-by: gabalafou <[email protected]>
This PR implements a new scrollspy mechanism to highlight entries in the secondary sidebar TOC; fixes #1965, and builds on top of #2107.
Approach
Every link in the secondary sidebar TOC is a reference to a section in the main article. This PR works by adding an
IntersectionObserver
that observes the heading at the top of every one of these sections. As the user scrolls down, theIntersectionObserver
triggers a callback when new section headings come into view; an object containing the visibility of the various sections is kept up to date by these events. The first visible heading is the one that needs to have its respective TOC entry highlighted.Other Notes
setupAnnouncementBanner
where aconst
was being assigned to. I fixed this just to make it stop complaining. If you'd rather have this in a separate PR, I'm happy to do so.IntersectionObserver
is set to observe element crossings with the viewport, but because there's a sticky menu bar at the top of the page I added arootMargin
to pad down the top edge so that element crossings happen below that menu bar. Testing by hand this seems to work pretty well.