feat: make navbar re-appear on upward scroll#8808
feat: make navbar re-appear on upward scroll#8808shivxmsharma wants to merge 1 commit intonodejs:mainfrom
Conversation
PR SummaryLow Risk Overview Adds a new Reviewed by Cursor Bugbot for commit b98e45d. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
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
Implements a “scroll-up-to-reveal” navbar behavior for the site by tracking scroll direction in a new client hook and toggling a CSS transform to hide/show the header area (desktop-only via xl: utilities).
Changes:
- Added
useScrollDirectionhook (client + server stub) with RAF-based scroll tracking and a threshold, plus unit tests. - Added
navBarWrapper/hiddenstyles to translate the header out of view on downward scroll atxlbreakpoint. - Integrated the hook into
WithNavBarto toggle the new wrapper classes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-components/src/Containers/NavBar/index.module.css | Adds navBarWrapper sticky + transform transition styles and a hidden state. |
| apps/site/hooks/client/useScrollDirection.ts | New client hook to detect scroll direction with RAF and a 10px threshold. |
| apps/site/hooks/client/tests/useScrollDirection.test.mjs | Unit tests for the new hook behavior. |
| apps/site/hooks/client/index.ts | Re-exports the new client hook. |
| apps/site/hooks/server/useScrollDirection.ts | Server stub to prevent calling the hook from RSC. |
| apps/site/hooks/server/index.ts | Re-exports the new server stub. |
| apps/site/components/withNavBar.tsx | Applies wrapper classes based on scroll direction. |
Comments suppressed due to low confidence (1)
apps/site/components/withNavBar.tsx:65
- This wrapper now contains
SkipToContentButton,WithBanner, andNavBar, but thenavBarWrapperstyling appliesxl:stickyand thehiddentransform to the entire wrapper. That means the banner (and skip link) will also become sticky and slide out of view on downward scroll, which doesn't match the PR description of hiding only the navbar and may reduce desktop screen real estate. Consider applying the wrapper class only around theNavBarelement (or splitting into separate wrappers) so banner behavior remains unchanged.
return (
<div
className={classNames(styles.navBarWrapper, {
[styles.hidden]: scrollDirection === 'down',
})}
>
<SkipToContentButton>
{t('components.common.skipToContent')}
</SkipToContentButton>
<WithBanner section="index" />
<NavBar
navItems={navigationItems.map(([, { label, link, target }]) => ({
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [scrollDirection, setScrollDirection] = useState<ScrollDirection>(null); | ||
| const lastScrollY = useRef(0); | ||
| const ticking = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| const updateScrollDirection = () => { | ||
| const currentScrollY = window.scrollY; | ||
|
|
||
| if (currentScrollY <= 0) { | ||
| setScrollDirection(null); | ||
| lastScrollY.current = currentScrollY; |
There was a problem hiding this comment.
lastScrollY is initialized to 0 and never set to the current window.scrollY on mount. If the page loads/restores at a non-zero scroll position (e.g., back/forward cache, anchor links), the first scroll event can mis-detect direction (e.g., scrolling up can still be reported as down). Initialize lastScrollY.current from window.scrollY when the effect runs (and consider keeping scrollDirection unchanged until you have a baseline).
| const onScroll = () => { | ||
| if (!ticking.current) { | ||
| ticking.current = true; | ||
| window.requestAnimationFrame(updateScrollDirection); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener('scroll', onScroll, { passive: true }); | ||
|
|
||
| return () => window.removeEventListener('scroll', onScroll); | ||
| }, []); |
There was a problem hiding this comment.
The requestAnimationFrame callback can still run after the component unmounts, which can call setScrollDirection on an unmounted component. Track the RAF id and cancel it in the cleanup (or guard updates with an isMounted ref) to avoid potential React warnings in fast route transitions.
| beforeEach(() => { | ||
| scrollY = 0; | ||
|
|
||
| Object.defineProperty(window, 'scrollY', { | ||
| get: () => scrollY, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| originalRAF = window.requestAnimationFrame; | ||
| Object.defineProperty(window, 'requestAnimationFrame', { | ||
| value: cb => { | ||
| cb(); | ||
| return 1; | ||
| }, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| window.requestAnimationFrame = originalRAF; | ||
| }); |
There was a problem hiding this comment.
This test overrides window.scrollY via Object.defineProperty but doesn't restore the original property descriptor in afterEach. That can leak into later tests and create order-dependent failures. Capture the original descriptor/value in beforeEach and restore it in afterEach (similar to how requestAnimationFrame is restored).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b98e45d. Configure here.
|
|
||
| const useScrollDirection = (): ScrollDirection => { | ||
| const [scrollDirection, setScrollDirection] = useState<ScrollDirection>(null); | ||
| const lastScrollY = useRef(0); |
There was a problem hiding this comment.
Incorrect direction on restored scroll position
Medium Severity
lastScrollY is initialized to 0 and never synced with the actual window.scrollY when the effect mounts. If the browser restores a non-zero scroll position (page refresh, back-navigation), the first scroll event compares against 0 instead of the real position. This causes scrolling up from, say, scrollY=500 to 490 to be reported as 'down' (since 490 > 0), incorrectly hiding the navbar. Initializing lastScrollY.current = window.scrollY at the start of the useEffect callback would fix this.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b98e45d. Configure here.
Description
This PR implements a "scroll-up-to-reveal" behavior for the navigation bar on the website, giving desktop users more screen real estate while scrolling through content.
When scrolling down, the navbar slides out of view. When scrolling up (or returning to the top of the page), it smoothly slides back into view.
Key implementation details:
useScrollDirectionclient hook that usesrequestAnimationFramefor high-performance, flicker-free scroll trackingSCROLL_THRESHOLDof 10px to avoid direction flips on micro-scrollsWithNavBarto apply ahiddenCSS class when scrolling downxlbreakpoint (≥1280px) only to avoid side effects on the mobile navigation layouttransform: translateY(-100%)for smooth, hardware-accelerated animationsValidation
scrollY === 0, the navbar is always visible regardless of previous scroll directionRelated Issues
Addresses #8699
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.