-
Notifications
You must be signed in to change notification settings - Fork 1.4k
docs: fix scroll position restoration when navigating back/forward #9242
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
Conversation
|
Build successful! 🎉 |
LFDanLu
left a 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.
Seems to work well, bit unfortunate that we have to do this manually though
packages/dev/s2-docs/src/client.tsx
Outdated
| let currentAbortController: AbortController | null = null; | ||
|
|
||
| // Store scroll positions by pathname | ||
| const scrollPositions = new Map<string, {scrollTop: number, windowScrollTop: number}>(); |
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.
Maybe we can save these in the history entry itself? https://stackoverflow.com/a/16732655
Might also have to turn off the browser's defaults? https://developer.chrome.com/blog/history-api-scroll-restoration
I did wonder if the defaults would work if we moved the key from the <main> element further down (e.g. article) so that it doesn't get replaced on each navigation. Did you try that?
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.
Tried moving the key and that didn't work.
packages/dev/s2-docs/src/client.tsx
Outdated
| if (scrollContainer) { | ||
| scrollContainer.scrollTop = position.scrollTop; | ||
| } | ||
| window.scrollTo(0, position.windowScrollTop); |
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.
Wouldn't it need to be the inner div that we scroll not the window?
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.
That one is for the mobile layout.
|
Build successful! 🎉 |
yihuiliao
left a 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.
seems to work well between the blog/releases, wasn't sure if there other places that are good to test
| scrollSaveTimeout = setTimeout(saveScrollPosition, 150); | ||
| } | ||
|
|
||
| window.addEventListener('scroll', onScroll, {passive: true, capture: true}); |
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.
do we need to watch it? or can we just grab scrollTop when we go to save? it looks like we already do that?
We need to handle this manually since we're using the scrollable container instead of the window.
✅ Pull Request Checklist:
📝 Test Instructions:
Verify that scroll position is restored when navigating back/forward in the browser. Also verify that scroll positions are still accurate for hash links. Also test on mobile.
🧢 Your Project: