FIX: Left scroll JS proper selector #534
Conversation
|
cc @jorisvandenbossche does this look good? |
| var active_pages = sidebarNav.querySelectorAll(".active"); | ||
| if (active_pages.length > 0) { | ||
| // Use the last active page as the offset since it's the page we're on | ||
| var offset = active_pages[active_pages.length - 1].offsetTop; |
There was a problem hiding this comment.
It's far in my memory, but the new code here is only taking the offsetTop from the last active item, while in the old code I am summing the OffsetTop values for all active elements. Was this change deliberate? If the original code was correct, this should give a change in behaviour? (didn't directly notice it though)
There was a problem hiding this comment.
Your memory is correct! I wasn't sure what the benefit was of calculating the offsetTop for all active items - don't we only ever care about the final active item (aka, the lowest one on the page), since that will be the page that we are on? What's the reason to calculate it for all of them?
There was a problem hiding this comment.
So I think this was done because offsetTop is relative to the parent div (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/offsetTop). And so because we have a set of nested divs here, if we want to know the actual, total offset of the page link (the final active item) to the top of the sidebar, we need to sum all those offsets (or find a property that gives this total directly).
Exploring this in the console of the browser for one of the sub-sub pages (https://pydata-sphinx-theme--534.org.readthedocs.build/en/534/demo/subpages/subsubpages/subsubpage5.html):
There was a problem hiding this comment.
(this might also be solvable with some position mangling which might determine which is the offset parent div .., but not directly understanding this)
There was a problem hiding this comment.
Ahh good point. I wonder if we could use this to be more efficient in the code:
https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect
There was a problem hiding this comment.
I think the problem is that that property gives the offset to the "viewport", so distance with the browser visible screen edge, while we need the offset to the actual page, so also what is hidden by scrolling down.
I would have thought that there was also some property to directly get that, but I didn't find anything back when doing the original PR.
There was a problem hiding this comment.
When you say "to the actual page", what do you mean? The just need the right information to set scrollTop equal to the right value, right? What do you think about the implementation here: 8ef7370
That calculates the y value of both the sidebar and the latest active item, and scrolls according to the diff
Co-authored-by: Damian Avila <damianavila82@yahoo.com.ar>
|
I think that this one is good to go? @jorisvandenbossche when we merge this one in, any objection to making a minor version release? |
|
OK I clarified the main question @damianavila had in his latest comment, will merge this one in unless there are objections, since this one is fixing a nasty bug! |
|
thanks all for the feedback! |
This is an attempt at getting the left scrollbar auto-scroll working again.
It simply adds an extra selector, and applies the offset to the parent div, not the nav div, since the parent div is the one that does the actual scrolling
fixes #535