feat (Navbar): add Navbar.center, shadow prop, hideOnScroll Prop, etc.#658
Open
paanSinghCoder wants to merge 20 commits intomainfrom
Open
feat (Navbar): add Navbar.center, shadow prop, hideOnScroll Prop, etc.#658paanSinghCoder wants to merge 20 commits intomainfrom
paanSinghCoder wants to merge 20 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Center navbar section, Changes
Sequence DiagramsequenceDiagram
participant ScrollContainer as Scroll Container
participant Window as Window
participant NavbarRoot as NavbarRoot
participant CSS as CSS Renderer
ScrollContainer->>Window: user scrolls
Window->>NavbarRoot: scroll event (or forwarded)
activate NavbarRoot
NavbarRoot->>NavbarRoot: compute direction & thresholds (lastY)
alt scrolled down past threshold
NavbarRoot->>NavbarRoot: set hidden = true
else scrolled up or near top
NavbarRoot->>NavbarRoot: set hidden = false
end
NavbarRoot->>CSS: set data-hidden="true"/"false" and data-shadow
CSS->>CSS: apply translateY transition and shadow rules (visual update)
deactivate NavbarRoot
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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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
- 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
rohanchkrabrty
requested changes
Mar 9, 2026
| align={align} | ||
| gap={gap} | ||
| className={cx(styles.center, className)} | ||
| role={ariaLabel ? 'group' : undefined} |
Contributor
There was a problem hiding this comment.
Let's make role="group" as default for Navbar parts. It shouldn't be conditional
Contributor
Author
There was a problem hiding this comment.
Conditional role='group' is intentional and according to ARIA guidelines The recommended pattern is to give that group an accessible name (e.g. via aria-label or aria-labelledby). A group without a name is often not very useful for assistive tech. https://www.w3.org/WAI/WCAG22/Techniques/aria/ARIA17
Contributor
There was a problem hiding this comment.
That holds true for grouping of form controls as mentioned in the above link.
We can use role='group' always in this case as it's a group of related items. Check MDN example here
| scope, | ||
| codePreview | ||
| codePreview, | ||
| previewClassName |
Contributor
There was a problem hiding this comment.
Let's not add previewClassName here to handle one off cases. It's better to use a container in demo.ts examples to achieve this.
It will by default show dotted content area irrespective of the component rendered.
Contributor
Author
There was a problem hiding this comment.
Removed the global previewClassName and added a wrapper around navbar.
Comment on lines
+24
to
+41
| function getScrollParent(el: HTMLElement | null): HTMLElement | null { | ||
| if (!el) return null; | ||
| let parent = el.parentElement; | ||
| while (parent) { | ||
| const { overflowY, overflow } = getComputedStyle(parent); | ||
| const scrollable = | ||
| overflowY === 'auto' || | ||
| overflowY === 'scroll' || | ||
| overflowY === 'overlay' || | ||
| overflow === 'auto' || | ||
| overflow === 'scroll' || | ||
| overflow === 'overlay'; | ||
| if (scrollable && parent.scrollHeight > parent.clientHeight) return parent; | ||
| parent = parent.parentElement; | ||
| } | ||
| return null; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
I think we can take a scrollContainerRef prop to get custom parent refs.
By default event listener will be attached on window, if user provides a scrollContainerRef that will be used instead.
Contributor
Author
There was a problem hiding this comment.
Adding scrollContainerRef does make sense but detecting and using immediate scroll parent instead of window makes sense as it works out of the box and matches user expectation.
Contributor
Author
There was a problem hiding this comment.
Added scrollContainerRef
Comment on lines
+90
to
+110
| const delta = scrollY - lastScrollY.current; | ||
| lastScrollY.current = scrollY; | ||
|
|
||
| if (scrollY <= SHOW_AT_TOP_THRESHOLD) { | ||
| setHidden(false); | ||
| accum.current = 0; | ||
| } else if (delta > 0) { | ||
| // Scrolling down: add to accum, reset if we were scrolling up | ||
| accum.current = accum.current > 0 ? accum.current + delta : delta; | ||
| if (accum.current >= SCROLL_THRESHOLD) { | ||
| setHidden(true); | ||
| accum.current = 0; | ||
| } | ||
| } else if (delta < 0) { | ||
| // Scrolling up: add to accum (delta is negative), reset if we were scrolling down | ||
| accum.current = accum.current < 0 ? accum.current + delta : delta; | ||
| if (accum.current <= -SCROLL_THRESHOLD) { | ||
| setHidden(false); | ||
| accum.current = 0; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
if (scrollY <= SHOW_AT_TOP_THRESHOLD) {
setHidden(false);
lastScrollY.current = scrollY;
} else if (scrollY > lastScrollY.current + SCROLL_THRESHOLD) {
setHidden(true);
lastScrollY.current = scrollY;
} else if (scrollY < lastScrollY.current - SCROLL_THRESHOLD) {
setHidden(false);
lastScrollY.current = scrollY;
}
If we only update lastScrollY when we are updating hidden state, then we don't need accumulator and delta. The logic can be greatly simplified
Comment on lines
+116
to
+132
| if (isWindow) { | ||
| if (typeof window === 'undefined') return; | ||
| lastScrollY.current = | ||
| window.scrollY ?? document.documentElement.scrollTop; | ||
| window.addEventListener('scroll', handleScroll, { passive: true }); | ||
| return () => window.removeEventListener('scroll', handleScroll); | ||
| } | ||
|
|
||
| if (scrollContainer) { | ||
| lastScrollY.current = scrollContainer.scrollTop; | ||
| scrollContainer.addEventListener('scroll', handleScroll, { | ||
| passive: true | ||
| }); | ||
| return () => | ||
| scrollContainer.removeEventListener('scroll', handleScroll); | ||
| } | ||
| }, [hideOnScroll]); |
Contributor
There was a problem hiding this comment.
since the listener and cleanup is same, let's reduce the code
element = isWindow? window : scrollContainer
element.addEventListener ...
return () => ...
Comment on lines
+135
to
+143
| const setRef = useCallback( | ||
| (node: HTMLElement | null) => { | ||
| navRef.current = node; | ||
| if (typeof ref === 'function') ref(node); | ||
| else if (ref) ref.current = node; | ||
| }, | ||
| [ref] | ||
| ); | ||
|
|
Contributor
There was a problem hiding this comment.
Let's use mergedRefs from base ui
import { useMergedRefs } from '@base-ui/utils/useMergedRefs';
const ref = useMergedRefs(refA, refB);
Contributor
Author
There was a problem hiding this comment.
Done. Added the new pkg @base-ui/utils to use the useMergedRefs. We will remove it in the future if it is not being used a lot.
| ref | ||
| ) => { | ||
| // hideOnScroll: whether navbar is currently hidden (driven by scroll position/direction) | ||
| const [hidden, setHidden] = useState(false); |
Contributor
There was a problem hiding this comment.
Let's skip the hidden state and handle it via the event listener ?
We have the navRef, we can set data state using js on the navRef
Contributor
Author
There was a problem hiding this comment.
Done. I don't see any major benefits of not using state here though apart from 1 extra re-render. Any other benefits you see?
Member
|
@rohanchkrabrty @paanSinghCoder Actually let's skip "Add a boolean prop shadow which toggles bottom shadow. Default to true." We can let it handle with className. This is as per suggestion by Rohan that we can avoid using props for custom styling. I think that makes sense. and default can be without shadow. |
Contributor
Author
|
@ravisuhag We control custom styling with props in a lot of other components as well. We will have to make a list and remove them as well. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/www/src/content/docs/components/navbar/demo.ts (1)
6-22: Consider extracting the repeated placeholder div style.The dashed-border placeholder div with identical inline styles appears in every demo tab. While acceptable for demo code, extracting this to a shared constant or CSS class would reduce repetition and make future styling changes easier.
♻️ Example: Extract placeholder style
// At the top of the file const placeholderStyle = `style={{ margin: 'var(--rs-space-8)', width: 'calc(100% - 2 * var(--rs-space-8))', minHeight: 200, border: '2px dashed var(--rs-color-border-base-secondary)', boxSizing: 'border-box' }}`; // Or define a constant object to interpolate const PLACEHOLDER_DIV = `<div style={{ margin: 'var(--rs-space-8)', width: 'calc(100% - 2 * var(--rs-space-8))', minHeight: 200, border: '2px dashed var(--rs-color-border-base-secondary)', boxSizing: 'border-box' }} />`;Then use template literals to compose demos with the shared placeholder.
Also applies to: 31-43, 71-81, 125-132
🤖 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 6 - 22, Extract the repeated inline style for the dashed placeholder div into a shared constant (e.g., placeholderStyle or PLACEHOLDER_DIV) at the top of apps/www/src/content/docs/components/navbar/demo.ts and replace each inline-styled placeholder div instances (the div with margin, calc width, minHeight: 200, dashed border and boxSizing) with a single reference to that constant or a CSS class; update all occurrences mentioned (around the blocks at lines referenced in the review) so future demos reuse the same style value.apps/www/src/components/demo/styles.module.css (1)
56-59: Configure Stylelint to recognize CSS Modules:globalpseudo-class.The
:global()pseudo-class is valid CSS Modules syntax for escaping module scope, but Stylelint flags it as unknown. Consider adding":global"to theselector-pseudo-class-no-unknownrule'signorePseudoClassesoption in your Stylelint configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/components/demo/styles.module.css` around lines 56 - 59, Stylelint is flagging the CSS Modules :global pseudo-class as unknown; update the Stylelint config (e.g., .stylelintrc.js or equivalent) to add ":global" to the selector-pseudo-class-no-unknown rule's ignorePseudoClasses array so selectors like .previewContentTop:has(> :global(.navbar-sticky-demo-scroll))::after are allowed; ensure the change references the rule name "selector-pseudo-class-no-unknown" and the pseudo-class string ":global".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/components/demo/styles.module.css`:
- Around line 27-59: Remove the dead CSS rules for .previewTop and
.previewContentTop (including the .previewContentTop::after block and the
.previewContentTop:has(> :global(.navbar-sticky-demo-scroll)) rule) since those
classes are not used by demo-preview, demo-playground, or demo-controls; either
delete those selectors from the styles.module.css or, if they are intended for
future use, add a clear TODO comment and a test/example that applies
.previewContentTop so the :has selector can match.
---
Nitpick comments:
In `@apps/www/src/components/demo/styles.module.css`:
- Around line 56-59: Stylelint is flagging the CSS Modules :global pseudo-class
as unknown; update the Stylelint config (e.g., .stylelintrc.js or equivalent) to
add ":global" to the selector-pseudo-class-no-unknown rule's ignorePseudoClasses
array so selectors like .previewContentTop:has(>
:global(.navbar-sticky-demo-scroll))::after are allowed; ensure the change
references the rule name "selector-pseudo-class-no-unknown" and the pseudo-class
string ":global".
In `@apps/www/src/content/docs/components/navbar/demo.ts`:
- Around line 6-22: Extract the repeated inline style for the dashed placeholder
div into a shared constant (e.g., placeholderStyle or PLACEHOLDER_DIV) at the
top of apps/www/src/content/docs/components/navbar/demo.ts and replace each
inline-styled placeholder div instances (the div with margin, calc width,
minHeight: 200, dashed border and boxSizing) with a single reference to that
constant or a CSS class; update all occurrences mentioned (around the blocks at
lines referenced in the review) so future demos reuse the same style value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a9010c1-62af-4d70-a0b9-5a7abdb174a3
📒 Files selected for processing (4)
apps/www/src/app/examples/page.tsxapps/www/src/components/demo/demo-preview.tsxapps/www/src/components/demo/styles.module.cssapps/www/src/content/docs/components/navbar/demo.ts
💤 Files with no reviewable changes (1)
- apps/www/src/components/demo/demo-preview.tsx
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/components/demo/styles.module.css (1)
202-205:⚠️ Potential issue | 🟠 MajorConflicting width/height declarations make the dialog fixed-size and non-responsive.
Line 204overridesLine 203, andLine 205overridesLine 202; the component ends up hard-fixed at1048x880, which can overflow smaller screens.Suggested responsive fix
.playgroundDialog { - height: 88vh; - width: 72%; - width: 1048px; - height: 880px; + width: min(72%, 1048px); + height: min(88vh, 880px); border-radius: var(--rs-radius-2); overflow: hidden; display: flex; flex-direction: column; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/components/demo/styles.module.css` around lines 202 - 205, The CSS has conflicting fixed and percentage width/height declarations causing a non-responsive 1048x880 dialog; remove the hard px overrides (width: 1048px and height: 880px) and instead use the percentage values with responsive limits—e.g., keep width: 72% and height: 88vh and add max-width: 1048px and max-height: 880px (or use min-width/min-height as appropriate) and optionally add media queries to adjust width/height on small screens; update the rules in apps/www/src/components/demo/styles.module.css where width/height are declared so the dialog scales rather than being fixed.apps/www/src/content/docs/components/navbar/demo.ts (1)
201-210:⚠️ Potential issue | 🟠 Major
aria-labelledbyneeds a real label target.
aria-labelledby="nav-heading"does not resolve to any element in this snippet, so the landmark ends up unnamed. That makes the “With aria-labelledby” example invalid and misleading.♿ Proposed fix
<div className="navbar-demo-wrapper"> - <Navbar aria-labelledby="nav-heading"> + <h2 id="navbar-demo-heading" style={{ margin: 'var(--rs-space-8)' }}> + Primary navigation + </h2> + <Navbar aria-labelledby="navbar-demo-heading"> <Navbar.Start>🤖 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 201 - 210, The demo uses aria-labelledby="nav-heading" on the Navbar but no element with id="nav-heading" exists, making the landmark unnamed; fix by adding a matching label element (e.g., a visually-hidden heading with id="nav-heading") inside the demo (for example within the Navbar wrapper or Navbar.Start) or alternatively replace aria-labelledby on Navbar with a proper aria-label value; ensure the element id exactly matches "nav-heading" so Navbar's aria-labelledby resolves.
♻️ Duplicate comments (1)
apps/www/src/components/demo/styles.module.css (1)
27-31:⚠️ Potential issue | 🟠 MajorStylelint is currently treating CSS Modules
:globalselectors as invalid.
Line 27andLine 63are valid for CSS Modules usage, but they failselector-pseudo-class-no-unknownin current lint settings. This will keep failing CI unless the rule is configured (or locally suppressed) for:global.#!/bin/bash set -euo pipefail echo "== Find stylelint config files ==" fd -H '(^|/)\.stylelintrc(\..*)?$|stylelint\.config\.(js|cjs|mjs|ts)$' echo "== Check selector-pseudo-class-no-unknown configuration ==" fd -H '(^|/)\.stylelintrc(\..*)?$|stylelint\.config\.(js|cjs|mjs|ts)$' \ | xargs -r rg -n "selector-pseudo-class-no-unknown|ignorePseudoClasses|global" echo "== Confirm problematic selectors in demo styles ==" rg -n "(:global\(|:has\()" apps/www/src/components/demo/styles.module.cssPossible in-file mitigation (if config change is not desired)
+/* stylelint-disable-next-line selector-pseudo-class-no-unknown */ .preview:has(:global(.navbar-demo-wrapper)) { align-items: flex-start; justify-content: flex-start; min-height: unset; } ... +/* stylelint-disable-next-line selector-pseudo-class-no-unknown */ .previewContentTop:has(> :global(.navbar-sticky-demo-scroll))::after { display: none; }Also applies to: 63-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/components/demo/styles.module.css` around lines 27 - 31, Stylelint is flagging CSS Modules pseudo-class syntax like .preview:has(:global(.navbar-demo-wrapper)); update the stylelint config rule "selector-pseudo-class-no-unknown" to allow CSS Modules pseudo-classes by adding ignorePseudoClasses entries for "global" (and "has" if needed), or alternatively add an inline stylelint suppression above the offending selectors (e.g., a single-line disable for selector-pseudo-class-no-unknown) in the demo styles file to bypass the rule for .preview:has(:global(.navbar-demo-wrapper)) and the other similar selector.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/content/docs/components/navbar/demo.ts`:
- Around line 6-22: The demo currently includes docs-only chrome (the outer div
with className "navbar-demo-wrapper" and the spacer div with inline style) which
ends up in the public snippet; remove those two wrapper elements from the
exported snippet so the sample only contains the actual component JSX (the
<Navbar> with <Navbar.Start> and <Navbar.End>, Search, and Buttons). Move the
wrapper/styling into the docs preview renderer (or story wrapper) so it applies
visually but is not part of the copied snippet; reference the className
"navbar-demo-wrapper" and the spacer div's inline style when updating the
preview renderer to re-add that chrome around the demo at render time. Ensure
the demo file (demo.ts) exports only the self-contained <Navbar> example.
In `@apps/www/src/content/docs/components/navbar/index.mdx`:
- Around line 48-52: The docs table for Navbar.Center is wrong — update the
props generator/source so NavbarCenterProps reflects the actual component props:
change the type exported in apps/www/src/content/docs/components/navbar/props.ts
from the current minimal shape (only aria-label) to match the implementation
which extends ComponentPropsWithoutRef<typeof Flex> in
packages/raystack/components/navbar/navbar-sections.tsx; ensure the generated
table includes Flex props such as align, gap, direction, etc., by importing or
deriving the correct type used by Navbar.Center (or re-exporting the real
NavbarCenterProps type) so the MDX <auto-type-table name="NavbarCenterProps" />
shows the complete API.
---
Outside diff comments:
In `@apps/www/src/components/demo/styles.module.css`:
- Around line 202-205: The CSS has conflicting fixed and percentage width/height
declarations causing a non-responsive 1048x880 dialog; remove the hard px
overrides (width: 1048px and height: 880px) and instead use the percentage
values with responsive limits—e.g., keep width: 72% and height: 88vh and add
max-width: 1048px and max-height: 880px (or use min-width/min-height as
appropriate) and optionally add media queries to adjust width/height on small
screens; update the rules in apps/www/src/components/demo/styles.module.css
where width/height are declared so the dialog scales rather than being fixed.
In `@apps/www/src/content/docs/components/navbar/demo.ts`:
- Around line 201-210: The demo uses aria-labelledby="nav-heading" on the Navbar
but no element with id="nav-heading" exists, making the landmark unnamed; fix by
adding a matching label element (e.g., a visually-hidden heading with
id="nav-heading") inside the demo (for example within the Navbar wrapper or
Navbar.Start) or alternatively replace aria-labelledby on Navbar with a proper
aria-label value; ensure the element id exactly matches "nav-heading" so
Navbar's aria-labelledby resolves.
---
Duplicate comments:
In `@apps/www/src/components/demo/styles.module.css`:
- Around line 27-31: Stylelint is flagging CSS Modules pseudo-class syntax like
.preview:has(:global(.navbar-demo-wrapper)); update the stylelint config rule
"selector-pseudo-class-no-unknown" to allow CSS Modules pseudo-classes by adding
ignorePseudoClasses entries for "global" (and "has" if needed), or alternatively
add an inline stylelint suppression above the offending selectors (e.g., a
single-line disable for selector-pseudo-class-no-unknown) in the demo styles
file to bypass the rule for .preview:has(:global(.navbar-demo-wrapper)) and the
other similar selector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bccdced-b7c7-4f59-9f2d-3ff4cd707e0b
📒 Files selected for processing (5)
apps/www/src/components/demo/styles.module.cssapps/www/src/content/docs/components/navbar/demo.cssapps/www/src/content/docs/components/navbar/demo.tsapps/www/src/content/docs/components/navbar/index.mdxpackages/raystack/components/navbar/navbar-root.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/raystack/components/navbar/navbar-root.tsx
Comment on lines
+48
to
+52
| ### Center | ||
|
|
||
| The center section sits in the middle of the navbar in a fixed position—it stays absolutely centered regardless of the width of Start or End content. The navbar uses a 3-column grid (Start | Center | End) so the center does not shift when left or right content changes. It accepts Flex props and section props. | ||
|
|
||
| <auto-type-table path="./props.ts" name="NavbarCenterProps" /> |
Contributor
There was a problem hiding this comment.
Navbar.Center's API table is out of sync with the implementation.
The prose says Navbar.Center accepts Flex props, but the generated table comes from apps/www/src/content/docs/components/navbar/props.ts:43-48, where NavbarCenterProps only documents aria-label. In packages/raystack/components/navbar/navbar-sections.tsx:8-40, the real type extends ComponentPropsWithoutRef<typeof Flex>, so align, gap, direction, etc. are currently missing from the reference.
🤖 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/index.mdx` around lines 48 - 52,
The docs table for Navbar.Center is wrong — update the props generator/source so
NavbarCenterProps reflects the actual component props: change the type exported
in apps/www/src/content/docs/components/navbar/props.ts from the current minimal
shape (only aria-label) to match the implementation which extends
ComponentPropsWithoutRef<typeof Flex> in
packages/raystack/components/navbar/navbar-sections.tsx; ensure the generated
table includes Flex props such as align, gap, direction, etc., by importing or
deriving the correct type used by Navbar.Center (or re-exporting the real
NavbarCenterProps type) so the MDX <auto-type-table name="NavbarCenterProps" />
shows the complete API.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/raystack/components/navbar/navbar-root.tsx (2)
123-131:⚠️ Potential issue | 🟠 MajorPreserve React 19 callback-ref cleanup semantics when merging refs.
This wrapper forwards object refs, but it drops any cleanup returned from a function ref. In React 19 that changes ref behavior; using
@base-ui/utils/useMergedRefshere avoids losing the forwarded ref’s cleanup.♻️ Suggested fix
+import { useMergedRefs } from '@base-ui/utils/useMergedRefs'; import { ComponentPropsWithoutRef, ComponentRef, forwardRef, - useCallback, useEffect, useRef, useState } from 'react'; @@ - const setRef = useCallback( - (node: HTMLElement | null) => { - navRef.current = node; - if (typeof ref === 'function') ref(node); - else if (ref) ref.current = node; - }, - [ref] - ); + const setRef = useMergedRefs(navRef, ref);In React 19, can callback refs return cleanup functions, and should a wrapper callback preserve that return value when forwarding refs?🤖 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 123 - 131, The wrapper setRef currently overwrites navRef and calls the forwarded ref but drops any cleanup returned by a function ref; update the merge to preserve React 19 callback-ref cleanup by replacing the custom useCallback merger with a proper merged ref utility (e.g., import and use `@base-ui/utils/useMergedRefs`) to combine navRef and the forwarded ref so any function ref's return value is preserved; update references to useMergedRefs for the mergedRef used in place of setRef (keep navRef, ref, and setRef usage sites consistent).
98-113:⚠️ Potential issue | 🟠 MajorKeep
hideOnScrollfrom hiding focused controls.This handler can still set
hidden=truewhile focus is inside the navbar, which slides focused inputs/buttons off-screen and breaks keyboard navigation. Bail out early whendocument.activeElementis contained bynavRef.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 98 - 113, The scroll handler handleScroll can still setHidden(true) while a control inside the navbar has focus; update handleScroll to bail out early when hide-on-scroll behavior should not run by checking if navRef.current exists and contains document.activeElement (i.e., if navRef.current.contains(document.activeElement) return) before any setHidden calls; ensure this check respects the existing hideOnScroll behavior (only apply the bail when hideOnScroll is enabled) and keep updates to lastScrollY.current unchanged so keyboard-focused controls are never slid off-screen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/content/docs/components/navbar/props.ts`:
- Around line 21-25: The type uses React.RefObject but there is no React import;
import the RefObject type directly from 'react' and update the prop type to use
RefObject instead of React.RefObject. Specifically, add an import for RefObject
from 'react' and change the scrollContainerRef declaration (symbol:
scrollContainerRef) to use RefObject<HTMLElement | null> so TypeScript (React
19) resolves the type correctly.
---
Duplicate comments:
In `@packages/raystack/components/navbar/navbar-root.tsx`:
- Around line 123-131: The wrapper setRef currently overwrites navRef and calls
the forwarded ref but drops any cleanup returned by a function ref; update the
merge to preserve React 19 callback-ref cleanup by replacing the custom
useCallback merger with a proper merged ref utility (e.g., import and use
`@base-ui/utils/useMergedRefs`) to combine navRef and the forwarded ref so any
function ref's return value is preserved; update references to useMergedRefs for
the mergedRef used in place of setRef (keep navRef, ref, and setRef usage sites
consistent).
- Around line 98-113: The scroll handler handleScroll can still setHidden(true)
while a control inside the navbar has focus; update handleScroll to bail out
early when hide-on-scroll behavior should not run by checking if navRef.current
exists and contains document.activeElement (i.e., if
navRef.current.contains(document.activeElement) return) before any setHidden
calls; ensure this check respects the existing hideOnScroll behavior (only apply
the bail when hideOnScroll is enabled) and keep updates to lastScrollY.current
unchanged so keyboard-focused controls are never slid off-screen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f66000a6-75c9-4759-b735-8abb7189bc29
📒 Files selected for processing (2)
apps/www/src/content/docs/components/navbar/props.tspackages/raystack/components/navbar/navbar-root.tsx
Comment on lines
+21
to
+25
| /** | ||
| * Ref to the scroll container. When provided (and current is set), used for hide-on-scroll | ||
| * instead of auto-detection. Otherwise the nearest scrollable ancestor or window is used. | ||
| */ | ||
| scrollContainerRef?: React.RefObject<HTMLElement | null>; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In a TypeScript module using React 19 typings, can React.RefObjectbe referenced without importingReact, or does that require allowUmdGlobalAccess / an explicit type import?
💡 Result:
No, in a TypeScript module using React 19 typings, React.RefObject cannot be referenced without importing React or using an explicit type import like import type { RefObject } from 'react'. It requires an explicit type import.
Citations:
- 1: https://stackoverflow.com/questions/79300987/react-19-ref-as-prop-and-typescript
- 2: https://medium.com/@ignatovich.dm/the-new-approach-to-passing-refs-in-react-19-typescript-a5762b938b93
- 3: [react] Types for React 19 DefinitelyTyped/DefinitelyTyped#69022
- 4: https://react.dev/blog/2024/04/25/react-19-upgrade-guide
🏁 Script executed:
# Check the actual imports in the file
cat -n apps/www/src/content/docs/components/navbar/props.ts | head -30
Repository: raystack/apsara
Length of output: 1089
🏁 Script executed:
# Check React version in the project
cat package.json | grep -A2 '"react"'
Repository: raystack/apsara
Length of output: 41
🏁 Script executed:
# Search for RefObject usage patterns in the codebase
rg 'RefObject' --type ts --type tsx -B2 -A2
Repository: raystack/apsara
Length of output: 86
🏁 Script executed:
# Check the full file to see if there are any imports
cat apps/www/src/content/docs/components/navbar/props.ts
Repository: raystack/apsara
Length of output: 1823
🏁 Script executed:
# Check package.json for React version
head -100 package.json | grep -E '"(react|version)"'
Repository: raystack/apsara
Length of output: 41
🏁 Script executed:
# Search for RefObject usage patterns (use ts separately)
rg 'RefObject' -t ts -B2 -A2
Repository: raystack/apsara
Length of output: 7139
Import RefObject from 'react' instead of using the React namespace.
Line 25 uses React.RefObject, but this file has no React import. This breaks type-checking in React 19's strict TypeScript setup. Add an explicit type import and reference RefObject directly.
🔧 Suggested fix
+import type { RefObject } from 'react';
+
export interface NavbarRootProps {
@@
- scrollContainerRef?: React.RefObject<HTMLElement | null>;
+ scrollContainerRef?: RefObject<HTMLElement | null>;
📝 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
| /** | |
| * Ref to the scroll container. When provided (and current is set), used for hide-on-scroll | |
| * instead of auto-detection. Otherwise the nearest scrollable ancestor or window is used. | |
| */ | |
| scrollContainerRef?: React.RefObject<HTMLElement | null>; | |
| import type { RefObject } from 'react'; | |
| /** | |
| * Ref to the scroll container. When provided (and current is set), used for hide-on-scroll | |
| * instead of auto-detection. Otherwise the nearest scrollable ancestor or window is used. | |
| */ | |
| scrollContainerRef?: RefObject<HTMLElement | null>; |
🤖 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/props.ts` around lines 21 - 25,
The type uses React.RefObject but there is no React import; import the RefObject
type directly from 'react' and update the prop type to use RefObject instead of
React.RefObject. Specifically, add an import for RefObject from 'react' and
change the scrollContainerRef declaration (symbol: scrollContainerRef) to use
RefObject<HTMLElement | null> so TypeScript (React 19) resolves the type
correctly.
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
3 participants
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)Add a boolean propRemoved shadow propshadowwhich 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
New Features
Documentation
Styling / UX
Tests