-
Notifications
You must be signed in to change notification settings - Fork 6.5k
feat: make navbar re-appear on upward scroll #8808
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { afterEach, beforeEach, describe, it } from 'node:test'; | ||
|
|
||
| import { renderHook, act } from '@testing-library/react'; | ||
|
|
||
| import useScrollDirection from '#site/hooks/client/useScrollDirection.js'; | ||
|
|
||
| describe('useScrollDirection', () => { | ||
| let scrollY; | ||
| let originalRAF; | ||
|
|
||
| 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; | ||
| }); | ||
|
|
||
| it('should return null initially (at top of page)', () => { | ||
| const { result } = renderHook(() => useScrollDirection()); | ||
| assert.equal(result.current, null); | ||
| }); | ||
|
|
||
| it('should return "down" when scrolling down past threshold', () => { | ||
| const { result } = renderHook(() => useScrollDirection()); | ||
|
|
||
| act(() => { | ||
| scrollY = 100; | ||
| window.dispatchEvent(new Event('scroll')); | ||
| }); | ||
|
|
||
| assert.equal(result.current, 'down'); | ||
| }); | ||
|
|
||
| it('should return "up" when scrolling up past threshold', () => { | ||
| const { result } = renderHook(() => useScrollDirection()); | ||
|
|
||
| act(() => { | ||
| scrollY = 100; | ||
| window.dispatchEvent(new Event('scroll')); | ||
| }); | ||
|
|
||
| act(() => { | ||
| scrollY = 50; | ||
| window.dispatchEvent(new Event('scroll')); | ||
| }); | ||
|
|
||
| assert.equal(result.current, 'up'); | ||
| }); | ||
|
|
||
| it('should not change direction for scroll less than threshold', () => { | ||
| const { result } = renderHook(() => useScrollDirection()); | ||
|
|
||
| act(() => { | ||
| scrollY = 5; | ||
| window.dispatchEvent(new Event('scroll')); | ||
| }); | ||
|
|
||
| assert.equal(result.current, null); | ||
| }); | ||
|
|
||
| it('should return null when scrolling back to top', () => { | ||
| const { result } = renderHook(() => useScrollDirection()); | ||
|
|
||
| act(() => { | ||
| scrollY = 100; | ||
| window.dispatchEvent(new Event('scroll')); | ||
| }); | ||
|
|
||
| assert.equal(result.current, 'down'); | ||
|
|
||
| act(() => { | ||
| scrollY = 0; | ||
| window.dispatchEvent(new Event('scroll')); | ||
| }); | ||
|
|
||
| assert.equal(result.current, null); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| 'use client'; | ||
|
|
||
| import { useState, useEffect, useRef } from 'react'; | ||
|
|
||
| type ScrollDirection = 'up' | 'down' | null; | ||
|
|
||
| const SCROLL_THRESHOLD = 10; | ||
|
|
||
| const useScrollDirection = (): ScrollDirection => { | ||
| const [scrollDirection, setScrollDirection] = useState<ScrollDirection>(null); | ||
| const lastScrollY = useRef(0); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect direction on restored scroll positionMedium Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit b98e45d. Configure here. |
||
| const ticking = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| const updateScrollDirection = () => { | ||
| const currentScrollY = window.scrollY; | ||
|
|
||
| if (currentScrollY <= 0) { | ||
| setScrollDirection(null); | ||
| lastScrollY.current = currentScrollY; | ||
|
Comment on lines
+10
to
+20
|
||
| ticking.current = false; | ||
| return; | ||
| } | ||
|
|
||
| const diff = Math.abs(currentScrollY - lastScrollY.current); | ||
|
|
||
| if (diff < SCROLL_THRESHOLD) { | ||
| ticking.current = false; | ||
| return; | ||
| } | ||
|
|
||
| setScrollDirection(currentScrollY > lastScrollY.current ? 'down' : 'up'); | ||
| lastScrollY.current = currentScrollY; | ||
| ticking.current = false; | ||
| }; | ||
|
|
||
| const onScroll = () => { | ||
| if (!ticking.current) { | ||
| ticking.current = true; | ||
| window.requestAnimationFrame(updateScrollDirection); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener('scroll', onScroll, { passive: true }); | ||
|
|
||
| return () => window.removeEventListener('scroll', onScroll); | ||
| }, []); | ||
|
Comment on lines
+37
to
+47
|
||
|
|
||
| return scrollDirection; | ||
| }; | ||
|
|
||
| export default useScrollDirection; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| export { default as useClientContext } from './useClientContext'; | ||
| export { default as useScrollToElement } from './useScrollToElement'; | ||
| export { default as useScroll } from './useScroll'; | ||
| export { default as useScrollDirection } from './useScrollDirection'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| const useScrollDirection = () => { | ||
| throw new Error('Attempted to call useScrollDirection from RSC'); | ||
| }; | ||
|
|
||
| export default useScrollDirection; |


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 test overrides
window.scrollYviaObject.definePropertybut doesn't restore the original property descriptor inafterEach. That can leak into later tests and create order-dependent failures. Capture the original descriptor/value inbeforeEachand restore it inafterEach(similar to howrequestAnimationFrameis restored).