feat (Navbar): add Navbar.center, shadow prop, hideOnScroll Prop, etc.#658
Open
paanSinghCoder wants to merge 11 commits intomainfrom
Open
feat (Navbar): add Navbar.center, shadow prop, hideOnScroll Prop, etc.#658paanSinghCoder wants to merge 11 commits intomainfrom
paanSinghCoder wants to merge 11 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds Navbar.Center section component and extends NavbarRoot with shadow and hideOnScroll props featuring scroll-based visibility toggling. Introduces previewTop preview layout styling for documentation demos. Updates Navbar documentation, demo configurations, and component implementations with comprehensive test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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.
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
Release Notes
New Features
Tests