feat(ui): implement sidebar scroll position preservation #8517
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
There was a problem hiding this comment.
Pull request overview
This PR implements scroll position preservation for the sidebar on the Learn Page, addressing issue #8336 where the sidebar would reset to the top when navigating between articles. The implementation uses React's forwardRef pattern and a custom hook to track and restore scroll positions across navigation.
Key Changes:
- Created a new
useNavigationStatehook that debounces scroll events and stores scroll positions in a context-based state store - Modified the
Sidebarcomponent to support ref forwarding, allowing parent components to access the underlying DOM element - Integrated the scroll preservation functionality into the
withSidebarcomponent
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
packages/ui-components/src/Containers/Sidebar/index.tsx |
Converted from functional component to forwardRef to expose the sidebar element for scroll position tracking |
apps/site/hooks/client/useNavigationState.ts |
New custom hook that manages scroll position state using context, debouncing, and DOM manipulation |
apps/site/hooks/client/index.ts |
Added export for the new useNavigationState hook |
apps/site/components/withSidebar.tsx |
Integrated scroll position preservation by using useNavigationState hook with a ref to the sidebar element |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…and fixed potential memory leak issue in useNavigationState
| import type { RefObject } from 'react'; | ||
|
|
||
| const useNavigationState = <T extends HTMLElement>( | ||
| id: string, |
There was a problem hiding this comment.
Do you need the ID if you have the ref or is the id used for something else?
There was a problem hiding this comment.
We do need id as multiple components might use this hook with different elements and the scroll positions are stored in the shared context object keyed by id.
| }, [id, ref, navigationState, debounceTime]); | ||
|
|
||
| useEffect(() => { | ||
| const element = ref.current; |
There was a problem hiding this comment.
Let's remove the redundant variable assignment. Let's use direct reference. I don't see why doing the assignment is necessary
| debounceTime = 300 | ||
| ) => { | ||
| const navigationState = useContext(NavigationStateContext); | ||
| const timeoutRef = useRef<NodeJS.Timeout | null>(null); |
There was a problem hiding this comment.
| const timeoutRef = useRef<NodeJS.Timeout | null>(null); | |
| const timeoutRef = useRef<NodeJS.Timeout>(); |
No need to have duality of null. By default a useRef .current will be undefined, so no need for explicit null value.
There was a problem hiding this comment.
TS overloads for useRef get a bit noisy without an explicit initial value, so I’m switching this to NodeJS.Timeout | undefined and initializing with undefined instead of null.
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
I feel we're missing tests that verify the hook works as expected and scrolls to an element that was outside of the view.
…handling in Sidebar
github-actions
bot
removed
the
github_actions:pull-request
|
Lighthouse Results
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8517 +/- ##
==========================================
- Coverage 74.70% 74.37% -0.34%
==========================================
Files 102 103 +1
Lines 8956 9012 +56
Branches 305 306 +1
==========================================
+ Hits 6691 6703 +12
- Misses 2263 2307 +44
Partials 2 2 ☔ View full report in Codecov by Sentry. |
…nt in Sidebar component
…r component and add scroll handling functionality
| }, [id, ref]); | ||
|
|
||
| // Save scroll position on scroll | ||
| const handleScroll = useCallback( |
There was a problem hiding this comment.
nit: not sure if this needs to be a callback
| if (savedState && savedState.y !== element.scrollTop) { | ||
| element.scroll({ top: savedState.y, behavior: 'auto' }); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
nit: add a comment explaining why only id and ref changes are enough.
| export { default as useDetectOS } from './useDetectOS'; | ||
| export { default as useMediaQuery } from './useMediaQuery'; | ||
| export { default as useClientContext } from './useClientContext'; | ||
| export { default as useScrollToElement } from './useScrollToElement'; |
There was a problem hiding this comment.
You also gotta make stub exports on hooks/server that when called throw exceptions. (see examples on hooks/server folder)
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
Left a few nits, but SGTM! And thanks for the work and effort here 🙇
3 participants
Description
This PR adds functionality to preserve sidebar scroll location on Learn Page
Validation
Before:
https://github.com/user-attachments/assets/41ca31eb-72c2-447a-a6a7-2d451171775e
After:
https://github.com/user-attachments/assets/656713ee-5d03-430f-beda-8ae491f415a8
Related Issues
Fixes #8336
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.