feat (Navbar): add Navbar.center, shadow prop, hideOnScroll Prop, etc.#658
Open
paanSinghCoder wants to merge 12 commits intomainfrom
Open
feat (Navbar): add Navbar.center, shadow prop, hideOnScroll Prop, etc.#658paanSinghCoder wants to merge 12 commits intomainfrom
paanSinghCoder wants to merge 12 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds Navbar.Center, shadow and hideOnScroll props (with scroll-driven hide behavior) to the Navbar; updates styling and tests; introduces previewTop demo layout via a new previewClassName prop and CSS for documentation demos; updates docs and demo exports. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Scrolling User
participant ScrollContainer as Scrollable Container (window/container)
participant NavbarRoot as NavbarRoot component
participant CSS as navbar.module.css
User->>ScrollContainer: scroll event
ScrollContainer->>NavbarRoot: onScroll handler
NavbarRoot->>NavbarRoot: compute direction & threshold, update `hidden` state
alt hidden = true
NavbarRoot->>CSS: set `data-hidden="true"`
CSS-->>NavbarRoot: transform: translateY(-var(--min-height))
else hidden = false
NavbarRoot->>CSS: set `data-hidden="false"`
CSS-->>NavbarRoot: transform: none
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Renamed demo section names for better clarity: 'Start Only' to 'Start', 'End Only' to 'End', and 'Both Sections' to 'Start, Center and End'. - Added a new demo section for 'Center' to showcase centered content in the Navbar. - Updated code snippets to reflect the new section names and structure.
- Added `previewClassName` prop to `DemoPreviewProps` for flexible styling. - Updated `DemoPreview` component to apply custom styles based on `previewClassName`. - Introduced new CSS classes for top-aligned preview content. - Updated demo documentation to utilize the new `previewClassName` feature.
- Added a new `shadowDemo` to showcase the Navbar with and without shadow. - Updated the sticky demo to include a scrollable container for better layout. - Modified CSS to hide the dashed box in sticky demos with internal scrollable content. - Updated documentation to reflect the new shadow feature in the Navbar.
- Introduced a new `hideOnScrollDemo` to demonstrate the Navbar's behavior when scrolling. - Updated the Navbar documentation to include the new demo, showcasing the navbar's visibility toggle on scroll. - Enhanced code snippets for better clarity and usability in the demo section.
- Updated the description of the `hideOnScroll` feature to specify that the slide animation is only visible when the navbar is sticky or fixed, enhancing user understanding of its functionality.
paanSinghCoder
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/www/src/content/docs/components/navbar/demo.ts (1)
178-189:⚠️ Potential issue | 🟡 Minor
aria-labelledbydemo is missing its referenced label element.The snippet references
nav-headingbut doesn’t render an element with that ID, so the example is incomplete/invalid for assistive tech users.💡 Proposed fix
<> + <Text id="nav-heading" size="small" weight="medium"> + Main navigation + </Text> <Navbar aria-labelledby="nav-heading">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/content/docs/components/navbar/demo.ts` around lines 178 - 189, The demo named "With aria-labelledby" references aria-labelledby="nav-heading" but never renders the referenced label; update the demo so there is a labeled element with id "nav-heading" (for example a visually-hidden heading or a Text component with id="nav-heading" and appropriate text) placed before or inside the <Navbar> so assistive tech can resolve the label; ensure the label text is meaningful and use the existing Navbar, Navbar.Start/End components in the changed snippet.packages/raystack/components/navbar/navbar-sections.tsx (1)
16-30:⚠️ Potential issue | 🟠 MajorAlso honor
aria-labelledbywhen assigningrole="group"on sections.Right now,
role="group"is only set foraria-label. If consumers usearia-labelledby, group semantics are not applied consistently. Please mirror the role condition for either accessible-name path across Start/Center/End.💡 Proposed fix pattern (apply to all three section components)
- 'aria-label': ariaLabel, + 'aria-label': ariaLabel, + 'aria-labelledby': ariaLabelledby, align = 'center', gap = 5, ...props @@ - role={ariaLabel ? 'group' : undefined} + role={ariaLabel || ariaLabelledby ? 'group' : undefined} aria-label={ariaLabel} + aria-labelledby={ariaLabelledby} {...props}Also applies to: 47-61, 77-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/navbar/navbar-sections.tsx` around lines 16 - 30, The Start/Center/End section components currently set role="group" only when ariaLabel is present; update the conditional to also check for ariaLabelledBy (e.g., use role={ariaLabel || ariaLabelledBy ? 'group' : undefined}) and ensure the aria-labelledby prop is forwarded (aria-labelledby={ariaLabelledBy}) alongside aria-label so both accessible-name paths trigger group semantics; apply this same change to all three section components (the blocks handling ariaLabel/role/props around the Flex element).
🧹 Nitpick comments (1)
packages/raystack/components/navbar/__tests__/navbar.test.tsx (1)
137-150:hideOnScrolltest currently validates only initial state, not scroll transitions.On Line 145, this suite confirms the default
data-hidden='false'state but doesn’t assert hide/reveal on downward/upward scroll, which is the core behavior.Suggested test expansion
describe('hideOnScroll', () => { it('sets data-hidden when hideOnScroll is true', () => { - render(<BasicNavbar hideOnScroll />); + render(<BasicNavbar hideOnScroll sticky />); const nav = screen.getByRole('navigation'); expect(nav).toHaveAttribute('data-hidden', 'false'); + + Object.defineProperty(window, 'scrollY', { configurable: true, value: 200 }); + window.dispatchEvent(new Event('scroll')); + expect(nav).toHaveAttribute('data-hidden', 'true'); + + Object.defineProperty(window, 'scrollY', { configurable: true, value: 50 }); + window.dispatchEvent(new Event('scroll')); + expect(nav).toHaveAttribute('data-hidden', 'false'); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/navbar/__tests__/navbar.test.tsx` around lines 137 - 150, The test only checks the initial data-hidden value; update the 'sets data-hidden when hideOnScroll is true' test to simulate user scrolling and assert transitions: render <BasicNavbar hideOnScroll />, ensure initial nav has data-hidden="false", then simulate a downward scroll (increase window.pageYOffset or mock scroll position and dispatch a window 'scroll' event) and assert the nav element (screen.getByRole('navigation')) has data-hidden="true", then simulate an upward scroll (decrease pageYOffset and dispatch another 'scroll') and assert data-hidden returns to "false"; reference the BasicNavbar component and the navigation element in your assertions and use testing-library's fireEvent or window.dispatchEvent for the scroll events.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/raystack/components/navbar/__tests__/navbar.test.tsx`:
- Around line 484-499: The test "positions Start, Center, and End correctly"
only asserts presence; either rename it to "renders Start, Center, and End
sections" to reflect the current checks, or add explicit position/order
assertions: locate the elements rendered by BasicNavbar (Navbar.Start,
Navbar.Center, Navbar.End) via container.querySelector / querySelectorAll with
styles.start, styles.center, styles.end and assert DOM order (e.g., verify the
NodeList order or use compareDocumentPosition) so the test name matches its
intent.
In `@packages/raystack/components/navbar/navbar-root.tsx`:
- Around line 69-82: The scroll handler (handleScroll) currently hides the
navbar via setHidden based only on scroll position and lastScrollY, which allows
the bar to disappear while a control inside it has focus; modify handleScroll to
detect if the navbar DOM contains document.activeElement (use the existing
navbar/root ref, e.g., navRef or rootRef) and, when focus is inside the navbar,
force visibility (call setHidden(false)) and skip hiding logic (still update
lastScrollY as needed). Ensure the check happens before the existing scroll
threshold checks so keyboard focus inside the navbar prevents hideOnScroll from
running.
---
Outside diff comments:
In `@apps/www/src/content/docs/components/navbar/demo.ts`:
- Around line 178-189: The demo named "With aria-labelledby" references
aria-labelledby="nav-heading" but never renders the referenced label; update the
demo so there is a labeled element with id "nav-heading" (for example a
visually-hidden heading or a Text component with id="nav-heading" and
appropriate text) placed before or inside the <Navbar> so assistive tech can
resolve the label; ensure the label text is meaningful and use the existing
Navbar, Navbar.Start/End components in the changed snippet.
In `@packages/raystack/components/navbar/navbar-sections.tsx`:
- Around line 16-30: The Start/Center/End section components currently set
role="group" only when ariaLabel is present; update the conditional to also
check for ariaLabelledBy (e.g., use role={ariaLabel || ariaLabelledBy ? 'group'
: undefined}) and ensure the aria-labelledby prop is forwarded
(aria-labelledby={ariaLabelledBy}) alongside aria-label so both accessible-name
paths trigger group semantics; apply this same change to all three section
components (the blocks handling ariaLabel/role/props around the Flex element).
---
Nitpick comments:
In `@packages/raystack/components/navbar/__tests__/navbar.test.tsx`:
- Around line 137-150: The test only checks the initial data-hidden value;
update the 'sets data-hidden when hideOnScroll is true' test to simulate user
scrolling and assert transitions: render <BasicNavbar hideOnScroll />, ensure
initial nav has data-hidden="false", then simulate a downward scroll (increase
window.pageYOffset or mock scroll position and dispatch a window 'scroll' event)
and assert the nav element (screen.getByRole('navigation')) has
data-hidden="true", then simulate an upward scroll (decrease pageYOffset and
dispatch another 'scroll') and assert data-hidden returns to "false"; reference
the BasicNavbar component and the navigation element in your assertions and use
testing-library's fireEvent or window.dispatchEvent for the scroll events.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/www/src/components/demo/demo-preview.tsxapps/www/src/components/demo/styles.module.cssapps/www/src/components/demo/types.tsapps/www/src/content/docs/components/navbar/demo.tsapps/www/src/content/docs/components/navbar/index.mdxapps/www/src/content/docs/components/navbar/props.tspackages/raystack/components/navbar/__tests__/navbar.test.tsxpackages/raystack/components/navbar/navbar-root.tsxpackages/raystack/components/navbar/navbar-sections.tsxpackages/raystack/components/navbar/navbar.module.csspackages/raystack/components/navbar/navbar.tsx
Comment on lines
+484
to
499
| it('positions Start, Center, and End correctly', () => { | ||
| const { container } = render( | ||
| <BasicNavbar> | ||
| <Navbar.Start data-testid='start'>Start</Navbar.Start> | ||
| <Navbar.Center data-testid='center'>Center</Navbar.Center> | ||
| <Navbar.End data-testid='end'>End</Navbar.End> | ||
| </BasicNavbar> | ||
| ); | ||
|
|
||
| const start = container.querySelector(`.${styles.start}`); | ||
| const center = container.querySelector(`.${styles.center}`); | ||
| const end = container.querySelector(`.${styles.end}`); | ||
|
|
||
| expect(start).toBeInTheDocument(); | ||
| expect(center).toBeInTheDocument(); | ||
| expect(end).toBeInTheDocument(); |
Contributor
There was a problem hiding this comment.
Test name and assertion intent are misaligned.
Line 484 says “positions ... correctly”, but the assertions only verify section presence. Either rename to reflect render presence or add order/position assertions.
Minimal alignment fix
-it('positions Start, Center, and End correctly', () => {
+it('renders Start, Center, and End sections', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('positions Start, Center, and End correctly', () => { | |
| const { container } = render( | |
| <BasicNavbar> | |
| <Navbar.Start data-testid='start'>Start</Navbar.Start> | |
| <Navbar.Center data-testid='center'>Center</Navbar.Center> | |
| <Navbar.End data-testid='end'>End</Navbar.End> | |
| </BasicNavbar> | |
| ); | |
| const start = container.querySelector(`.${styles.start}`); | |
| const center = container.querySelector(`.${styles.center}`); | |
| const end = container.querySelector(`.${styles.end}`); | |
| expect(start).toBeInTheDocument(); | |
| expect(center).toBeInTheDocument(); | |
| expect(end).toBeInTheDocument(); | |
| it('renders Start, Center, and End sections', () => { | |
| const { container } = render( | |
| <BasicNavbar> | |
| <Navbar.Start data-testid='start'>Start</Navbar.Start> | |
| <Navbar.Center data-testid='center'>Center</Navbar.Center> | |
| <Navbar.End data-testid='end'>End</Navbar.End> | |
| </BasicNavbar> | |
| ); | |
| const start = container.querySelector(`.${styles.start}`); | |
| const center = container.querySelector(`.${styles.center}`); | |
| const end = container.querySelector(`.${styles.end}`); | |
| expect(start).toBeInTheDocument(); | |
| expect(center).toBeInTheDocument(); | |
| expect(end).toBeInTheDocument(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/navbar/__tests__/navbar.test.tsx` around lines
484 - 499, The test "positions Start, Center, and End correctly" only asserts
presence; either rename it to "renders Start, Center, and End sections" to
reflect the current checks, or add explicit position/order assertions: locate
the elements rendered by BasicNavbar (Navbar.Start, Navbar.Center, Navbar.End)
via container.querySelector / querySelectorAll with styles.start, styles.center,
styles.end and assert DOM order (e.g., verify the NodeList order or use
compareDocumentPosition) so the test name matches its intent.
Comment on lines
+69
to
+82
| const handleScroll = () => { | ||
| if (ticking) return; | ||
| ticking = true; | ||
| requestAnimationFrame(() => { | ||
| const scrollY = isWindow | ||
| ? (window.scrollY ?? document.documentElement.scrollTop) | ||
| : scrollContainer!.scrollTop; | ||
| if (scrollY <= SHOW_AT_TOP_THRESHOLD) { | ||
| setHidden(false); | ||
| } else if (scrollY > lastScrollY.current + SCROLL_THRESHOLD) { | ||
| setHidden(true); | ||
| } else if (scrollY < lastScrollY.current - SCROLL_THRESHOLD) { | ||
| setHidden(false); | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent hiding while navbar contains focus.
hideOnScroll can move the navbar off-screen even when one of its controls is focused, which breaks keyboard navigation and focus visibility. Keep it visible while focus is within the navbar.
💡 Proposed fix
requestAnimationFrame(() => {
const scrollY = isWindow
? (window.scrollY ?? document.documentElement.scrollTop)
: scrollContainer!.scrollTop;
+ const activeEl = document.activeElement;
+ if (activeEl && el.contains(activeEl)) {
+ setHidden(false);
+ lastScrollY.current = scrollY;
+ ticking = false;
+ return;
+ }
if (scrollY <= SHOW_AT_TOP_THRESHOLD) {
setHidden(false);
} else if (scrollY > lastScrollY.current + SCROLL_THRESHOLD) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleScroll = () => { | |
| if (ticking) return; | |
| ticking = true; | |
| requestAnimationFrame(() => { | |
| const scrollY = isWindow | |
| ? (window.scrollY ?? document.documentElement.scrollTop) | |
| : scrollContainer!.scrollTop; | |
| if (scrollY <= SHOW_AT_TOP_THRESHOLD) { | |
| setHidden(false); | |
| } else if (scrollY > lastScrollY.current + SCROLL_THRESHOLD) { | |
| setHidden(true); | |
| } else if (scrollY < lastScrollY.current - SCROLL_THRESHOLD) { | |
| setHidden(false); | |
| } | |
| const handleScroll = () => { | |
| if (ticking) return; | |
| ticking = true; | |
| requestAnimationFrame(() => { | |
| const scrollY = isWindow | |
| ? (window.scrollY ?? document.documentElement.scrollTop) | |
| : scrollContainer!.scrollTop; | |
| const activeEl = document.activeElement; | |
| if (activeEl && el.contains(activeEl)) { | |
| setHidden(false); | |
| lastScrollY.current = scrollY; | |
| ticking = false; | |
| return; | |
| } | |
| if (scrollY <= SHOW_AT_TOP_THRESHOLD) { | |
| setHidden(false); | |
| } else if (scrollY > lastScrollY.current + SCROLL_THRESHOLD) { | |
| setHidden(true); | |
| } else if (scrollY < lastScrollY.current - SCROLL_THRESHOLD) { | |
| setHidden(false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/navbar/navbar-root.tsx` around lines 69 - 82,
The scroll handler (handleScroll) currently hides the navbar via setHidden based
only on scroll position and lastScrollY, which allows the bar to disappear while
a control inside it has focus; modify handleScroll to detect if the navbar DOM
contains document.activeElement (use the existing navbar/root ref, e.g., navRef
or rootRef) and, when focus is inside the navbar, force visibility (call
setHidden(false)) and skip hiding logic (still update lastScrollY as needed).
Ensure the check happens before the existing scroll threshold checks so keyboard
focus inside the navbar prevents hideOnScroll from running.
- Updated the `SCROLL_THRESHOLD` to improve responsiveness when hiding/showing the navbar on scroll. - Added detailed documentation comments for the `NavbarRoot` component and its props, including `sticky`, `shadow`, and `hideOnScroll`. - Implemented a more refined scroll detection mechanism to handle slow scrolls effectively, ensuring smoother visibility toggling of the navbar.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/raystack/components/navbar/navbar.module.css (1)
8-9: Consider respectingprefers-reduced-motionfor the hide/show transition.Users who prefer reduced motion may find the sliding animation distracting. The transition can be disabled or shortened for accessibility.
♻️ Suggested enhancement
.root { display: flex; width: 100%; padding: var(--rs-space-4) var(--rs-space-7); background: var(--rs-color-background-base-primary); border-bottom: 0.5px solid var(--rs-color-border-base-primary); box-sizing: border-box; min-height: var(--rs-space-11); transition: transform 0.2s ease-in-out; } + +@media (prefers-reduced-motion: reduce) { + .root { + transition: none; + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/navbar/navbar.module.css` around lines 8 - 9, The navbar CSS currently applies a transform transition ("transition: transform 0.2s ease-in-out;") which ignores users' reduced-motion preferences; update the stylesheet to respect prefers-reduced-motion by adding a media query for (prefers-reduced-motion: reduce) that disables or drastically shortens the transform transition for the navbar rule (the rule containing "transition: transform 0.2s ease-in-out") so the hide/show animation is removed or minimized for users who opt out of motion.packages/raystack/components/navbar/navbar-root.tsx (2)
135-142: Ref merging implementation works but consider using a utility.The manual ref merging handles both function and object refs correctly. If the codebase has a
mergeRefsutility (or could add one), it would simplify this pattern and ensure consistency across components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/navbar/navbar-root.tsx` around lines 135 - 142, Replace the manual ref-merging logic in the setRef callback with the shared mergeRefs utility: locate the setRef function that currently assigns navRef.current and then conditionally calls or assigns ref (symbols: setRef, navRef, ref), and change it to use mergeRefs([navRef, ref]) when creating the merged ref (or call mergeRefs in the component's ref prop) so the shared utility handles both function and object refs consistently and eliminates the custom conditional logic.
116-122: SSR guard placement may cause issues.The
typeof window === 'undefined'check on line 117 occurs afterisWindowis already computed usinggetScrollParent. WhileuseEffectdoesn't run on the server, this check would be cleaner at the effect's start for clarity and defensive coding.♻️ Suggested improvement
useEffect(() => { if (!hideOnScroll) return; + if (typeof window === 'undefined') return; const el = navRef.current; if (!el) return; const scrollContainer = getScrollParent(el); const isWindow = scrollContainer === null; // ... if (isWindow) { - if (typeof window === 'undefined') return; lastScrollY.current =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/navbar/navbar-root.tsx` around lines 116 - 122, Move the server-side guard "typeof window === 'undefined'" to the top of the useEffect callback so we never reference client-only values before confirming we're in the browser; specifically, check "typeof window === 'undefined'" at the start of the effect and return early, then compute or use "isWindow" (from getScrollParent) and proceed to set "lastScrollY.current", add the "scroll" listener with "handleScroll", and return the cleanup that removes it. This ensures getScrollParent/isWindow and window.scrollY are only accessed on the client.packages/raystack/components/navbar/__tests__/navbar.test.tsx (1)
137-151: Consider adding scroll behavior integration tests.These tests verify initial attribute states but don't test the actual scroll-triggered hide/show behavior. While unit tests may not be the right place for scroll simulation, consider adding integration tests or documenting that scroll behavior is manually tested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/navbar/__tests__/navbar.test.tsx` around lines 137 - 151, Add an integration test that verifies the scroll-triggered hide/show behavior for the BasicNavbar component when hideOnScroll is true: render <BasicNavbar hideOnScroll />, programmatically dispatch scroll events that move window.scrollY past the threshold used by the component, and assert that the navigation element (queried via getByRole('navigation')) toggles its data-hidden attribute accordingly (e.g., becomes 'true' when scrolled down and 'false' when scrolled up); if an integration test environment is not appropriate, add a clear test-file comment documenting that scroll behavior is covered by manual or E2E tests and reference the hideOnScroll prop and data-hidden attribute for traceability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/raystack/components/navbar/__tests__/navbar.test.tsx`:
- Around line 137-151: Add an integration test that verifies the
scroll-triggered hide/show behavior for the BasicNavbar component when
hideOnScroll is true: render <BasicNavbar hideOnScroll />, programmatically
dispatch scroll events that move window.scrollY past the threshold used by the
component, and assert that the navigation element (queried via
getByRole('navigation')) toggles its data-hidden attribute accordingly (e.g.,
becomes 'true' when scrolled down and 'false' when scrolled up); if an
integration test environment is not appropriate, add a clear test-file comment
documenting that scroll behavior is covered by manual or E2E tests and reference
the hideOnScroll prop and data-hidden attribute for traceability.
In `@packages/raystack/components/navbar/navbar-root.tsx`:
- Around line 135-142: Replace the manual ref-merging logic in the setRef
callback with the shared mergeRefs utility: locate the setRef function that
currently assigns navRef.current and then conditionally calls or assigns ref
(symbols: setRef, navRef, ref), and change it to use mergeRefs([navRef, ref])
when creating the merged ref (or call mergeRefs in the component's ref prop) so
the shared utility handles both function and object refs consistently and
eliminates the custom conditional logic.
- Around line 116-122: Move the server-side guard "typeof window ===
'undefined'" to the top of the useEffect callback so we never reference
client-only values before confirming we're in the browser; specifically, check
"typeof window === 'undefined'" at the start of the effect and return early,
then compute or use "isWindow" (from getScrollParent) and proceed to set
"lastScrollY.current", add the "scroll" listener with "handleScroll", and return
the cleanup that removes it. This ensures getScrollParent/isWindow and
window.scrollY are only accessed on the client.
In `@packages/raystack/components/navbar/navbar.module.css`:
- Around line 8-9: The navbar CSS currently applies a transform transition
("transition: transform 0.2s ease-in-out;") which ignores users' reduced-motion
preferences; update the stylesheet to respect prefers-reduced-motion by adding a
media query for (prefers-reduced-motion: reduce) that disables or drastically
shortens the transform transition for the navbar rule (the rule containing
"transition: transform 0.2s ease-in-out") so the hide/show animation is removed
or minimized for users who opt out of motion.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/www/src/app/examples/page.tsxapps/www/src/content/docs/components/navbar/index.mdxpackages/raystack/components/navbar/__tests__/navbar.test.tsxpackages/raystack/components/navbar/navbar-root.tsxpackages/raystack/components/navbar/navbar.module.css
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
issue-576
var(--space-11)shadowwhich toggles bottom shadow. Default to true.hideOnScrollprop.<Navbar.Center />will be absolutely horizontally centered in the Navbar.Flex. Added defaultgap={5}according to the design requirement.role='group'is intentional and according to ARIA guidelines The recommended pattern is to give that group an accessible name (e.g. viaaria-labeloraria-labelledby). A group without a name is often not very useful for assistive tech. ARIAflex: displayto container.Type of Change
How Has This Been Tested?
Manually on examples page
Checklist:
Summary by CodeRabbit