feat (sidebar): add separator for section header in collapsed state#720
Open
paanSinghCoder wants to merge 4 commits intomainfrom
Open
feat (sidebar): add separator for section header in collapsed state#720paanSinghCoder wants to merge 4 commits intomainfrom
paanSinghCoder wants to merge 4 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
paanSinghCoder
changed the title
paanSinghCoder
changed the title
paanSinghCoder
changed the title
paanSinghCoder
changed the title
Contributor
📝 WalkthroughWalkthroughAdds a new Sidebar.More compound component, a SidebarLeadingVisual helper, a variant prop for SidebarRoot, variant/inset/floating styles, expanded CSS for collapsed headers and "more" menu styling, documentation/demo updates, tests for variants and More, and updates example usages to Radix icons and new Sidebar APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Page / Consumer
participant Sidebar as SidebarRoot (Context)
participant More as Sidebar.More (Trigger)
participant Menu as Menu (Dropdown)
participant Item as Sidebar.Item
Page->>Sidebar: render (variant, position, open)
Sidebar->>Sidebar: provide context {isCollapsed, position, hideCollapsedItemTooltip}
Page->>More: render More with children items
More->>Sidebar: read context (isCollapsed, position)
More->>More: build trigger (leadingIcon or default)
alt sidebar collapsed and tooltip allowed
More->>Menu: wrap Trigger in Tooltip -> open
else expanded or tooltip suppressed
More->>Menu: render Trigger directly -> open
end
Menu->>Item: render Menu.Item for each child (clone item props, leading visual, label)
Item-->>Page: user selects item -> navigation/action
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…les and documentation
![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: 1
🧹 Nitpick comments (4)
apps/www/src/app/examples/page.tsx (1)
139-146: Consider adding a label for accessibility.
Sidebar.Morewithout alabelprop (onlyleadingIcon) may lack a descriptive accessible name in collapsed state when tooltips are shown.♿ Optional: Add label for better accessibility
- <Sidebar.More leadingIcon={<DotsHorizontalIcon />}> + <Sidebar.More label="More options" leadingIcon={<DotsHorizontalIcon />}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/www/src/app/examples/page.tsx` around lines 139 - 146, Sidebar.More currently only provides a leadingIcon (<DotsHorizontalIcon />) and lacks an accessible label; add a descriptive label prop to Sidebar.More (e.g., label="More") so screen readers and collapsed/tooltip states have a clear name. Update the Sidebar.More component usage in apps/www/src/app/examples/page.tsx to pass a concise label string alongside leadingIcon, and ensure any tooltip logic uses that label if applicable.packages/raystack/components/sidebar/__tests__/sidebar.test.tsx (1)
290-322: Good test coverage forSidebar.More.The tests cover both expanded and collapsed states with proper accessibility assertions. Consider using
screen.getByRole('button', { name: 'More items' })for a more robust trigger selection instead of.closest('button').♻️ Optional: Use role-based query for trigger
- const trigger = screen.getByText('More items').closest('button'); - expect(trigger).toBeInTheDocument(); - if (!trigger) return; - fireEvent.click(trigger); + const trigger = screen.getByRole('button', { name: /More items/i }); + fireEvent.click(trigger);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/sidebar/__tests__/sidebar.test.tsx` around lines 290 - 322, Replace the fragile DOM traversal used to find the More trigger in the 'Sidebar More' test: instead of locating the label text and calling .closest('button') (see the test that uses screen.getByText('More items').closest('button')), use a role-based query like screen.getByRole('button', { name: 'More items' }) to reliably select the trigger; remove the subsequent if (!trigger) return guard and assert directly that the returned element exists before firing events.packages/raystack/components/sidebar/sidebar.module.css (1)
259-267: Minor formatting inconsistency: extra blank line inside media query.There's an extra blank line at line 260 inside the
@media (prefers-reduced-motion: reduce)block that differs from the typical formatting in this file.🔧 Suggested fix
`@media` (prefers-reduced-motion: reduce) { - .root, .nav-item, .nav-text, .resizeHandle { transition: none; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/sidebar/sidebar.module.css` around lines 259 - 267, Remove the extra blank line inside the `@media` (prefers-reduced-motion: reduce) block so its formatting matches the rest of the file; specifically, collapse the blank line between the media query declaration and the selector list that includes .root, .nav-item, .nav-text, and .resizeHandle so the block reads as a contiguous rule set without an empty line.packages/raystack/components/sidebar/sidebar-more.tsx (1)
90-143: Consider theitem.keyfallback behavior.Using
item.key ?? indexat line 138 is functional, but React'skeyprop on a component is only accessible viaitem.keywhen iterating over children. If consumers don't provide explicit keys toSidebarItemchildren, all items will use index-based keys, which can cause issues with list reordering or dynamic items.This is acceptable for typical sidebar menus with static items, but consider documenting that consumers should provide unique keys for dynamic item lists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/sidebar/sidebar-more.tsx` around lines 90 - 143, The current key fallback uses item.key ?? index which can cause problems for reordering; update the key selection in the items.map -> Menu.Item by preferring an explicit unique identifier before falling back to index (e.g., check item.key, then item.props.id or another stable identifier on the child) and add a short developer-facing note/documentation near SidebarMore (or SidebarItem) explaining that consumers should provide explicit keys/ids for dynamic lists to avoid reconciliation issues.
🤖 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/sidebar/sidebar-more.tsx`:
- Around line 51-72: The button element inside triggerContent currently has
role="listitem", which conflicts with its function as a menu trigger; remove the
explicit role attribute from the button in sidebar-more.tsx so native button
semantics apply, or if this is intended to be presented as a menu item use
role="menuitem" (and add aria-haspopup/aria-expanded as appropriate) instead;
update the JSX where triggerContent is defined (the <button> that renders
SidebarLeadingVisual and label) accordingly.
---
Nitpick comments:
In `@apps/www/src/app/examples/page.tsx`:
- Around line 139-146: Sidebar.More currently only provides a leadingIcon
(<DotsHorizontalIcon />) and lacks an accessible label; add a descriptive label
prop to Sidebar.More (e.g., label="More") so screen readers and
collapsed/tooltip states have a clear name. Update the Sidebar.More component
usage in apps/www/src/app/examples/page.tsx to pass a concise label string
alongside leadingIcon, and ensure any tooltip logic uses that label if
applicable.
In `@packages/raystack/components/sidebar/__tests__/sidebar.test.tsx`:
- Around line 290-322: Replace the fragile DOM traversal used to find the More
trigger in the 'Sidebar More' test: instead of locating the label text and
calling .closest('button') (see the test that uses screen.getByText('More
items').closest('button')), use a role-based query like
screen.getByRole('button', { name: 'More items' }) to reliably select the
trigger; remove the subsequent if (!trigger) return guard and assert directly
that the returned element exists before firing events.
In `@packages/raystack/components/sidebar/sidebar-more.tsx`:
- Around line 90-143: The current key fallback uses item.key ?? index which can
cause problems for reordering; update the key selection in the items.map ->
Menu.Item by preferring an explicit unique identifier before falling back to
index (e.g., check item.key, then item.props.id or another stable identifier on
the child) and add a short developer-facing note/documentation near SidebarMore
(or SidebarItem) explaining that consumers should provide explicit keys/ids for
dynamic lists to avoid reconciliation issues.
In `@packages/raystack/components/sidebar/sidebar.module.css`:
- Around line 259-267: Remove the extra blank line inside the `@media`
(prefers-reduced-motion: reduce) block so its formatting matches the rest of the
file; specifically, collapse the blank line between the media query declaration
and the selector list that includes .root, .nav-item, .nav-text, and
.resizeHandle so the block reads as a contiguous rule set without an empty line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 399fa010-1e7a-4d64-a652-02526abcd1cd
📒 Files selected for processing (11)
apps/www/src/app/examples/page.tsxapps/www/src/content/docs/components/sidebar/demo.tsapps/www/src/content/docs/components/sidebar/index.mdxapps/www/src/content/docs/components/sidebar/props.tspackages/raystack/components/sidebar/__tests__/sidebar.test.tsxpackages/raystack/components/sidebar/sidebar-item.tsxpackages/raystack/components/sidebar/sidebar-leading-visual.tsxpackages/raystack/components/sidebar/sidebar-more.tsxpackages/raystack/components/sidebar/sidebar-root.tsxpackages/raystack/components/sidebar/sidebar.module.csspackages/raystack/components/sidebar/sidebar.tsx
Comment on lines
+51
to
+72
| const triggerContent = ( | ||
| <button | ||
| type='button' | ||
| className={cx( | ||
| styles['nav-item'], | ||
| styles['more-trigger'], | ||
| classNames?.root | ||
| )} | ||
| role='listitem' | ||
| aria-label={isCollapsed ? label : undefined} | ||
| > | ||
| <SidebarLeadingVisual | ||
| leadingIcon={triggerIcon} | ||
| className={classNames?.leadingIcon} | ||
| /> | ||
| {!isCollapsed && ( | ||
| <span className={cx(styles['nav-text'], classNames?.text)}> | ||
| {label} | ||
| </span> | ||
| )} | ||
| </button> | ||
| ); |
Contributor
There was a problem hiding this comment.
Semantic role mismatch: role="listitem" on a menu trigger button.
The button has role="listitem", but it functions as a menu trigger. When the Menu component renders, it likely applies aria-haspopup and manages focus as a menu button. The listitem role may conflict with menu semantics and confuse assistive technologies.
Consider removing the explicit role or using a more appropriate role like role="menuitem" if this is intended to be part of a navigation list structure.
🛡️ Suggested fix
<button
type='button'
className={cx(
styles['nav-item'],
styles['more-trigger'],
classNames?.root
)}
- role='listitem'
aria-label={isCollapsed ? label : undefined}
>📝 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 triggerContent = ( | |
| <button | |
| type='button' | |
| className={cx( | |
| styles['nav-item'], | |
| styles['more-trigger'], | |
| classNames?.root | |
| )} | |
| role='listitem' | |
| aria-label={isCollapsed ? label : undefined} | |
| > | |
| <SidebarLeadingVisual | |
| leadingIcon={triggerIcon} | |
| className={classNames?.leadingIcon} | |
| /> | |
| {!isCollapsed && ( | |
| <span className={cx(styles['nav-text'], classNames?.text)}> | |
| {label} | |
| </span> | |
| )} | |
| </button> | |
| ); | |
| const triggerContent = ( | |
| <button | |
| type='button' | |
| className={cx( | |
| styles['nav-item'], | |
| styles['more-trigger'], | |
| classNames?.root | |
| )} | |
| aria-label={isCollapsed ? label : undefined} | |
| > | |
| <SidebarLeadingVisual | |
| leadingIcon={triggerIcon} | |
| className={classNames?.leadingIcon} | |
| /> | |
| {!isCollapsed && ( | |
| <span className={cx(styles['nav-text'], classNames?.text)}> | |
| {label} | |
| </span> | |
| )} | |
| </button> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/sidebar/sidebar-more.tsx` around lines 51 - 72,
The button element inside triggerContent currently has role="listitem", which
conflicts with its function as a menu trigger; remove the explicit role
attribute from the button in sidebar-more.tsx so native button semantics apply,
or if this is intended to be presented as a menu item use role="menuitem" (and
add aria-haspopup/aria-expanded as appropriate) instead; update the JSX where
triggerContent is defined (the <button> that renders SidebarLeadingVisual and
label) accordingly.
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
Type of Change
How Has This Been Tested?
[Describe the tests that you ran to verify your changes]
Checklist:
Screenshots (if appropriate):
[Add screenshots here]
Related Issues
[Link any related issues here using #issue-number]
Summary by CodeRabbit
New Features
Style
Documentation