feat (sidebar): add variants, more section menu and separator for section header in collapsed state#720
feat (sidebar): add variants, more section menu and separator for section header in collapsed state#720paanSinghCoder wants to merge 10 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
|
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 Sidebar.More and SidebarLeadingVisual components; introduces a Sequence Diagram(s)sequenceDiagram
participant Page as Page / Consumer
participant Sidebar as SidebarRoot (Context)
participant More as Sidebar.More (Trigger)
participant Tooltip as Tooltip (optional)
participant Menu as Menu (Dropdown)
participant Item as Sidebar.Item
Page->>Sidebar: render (props: open, position, variant, hideCollapsedItemTooltip)
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 DotsHorizontalIcon)
alt isCollapsed && tooltip allowed
More->>Tooltip: wrap trigger in Tooltip
Tooltip->>Menu: on trigger open -> show Menu positioned by Sidebar.position
else
More->>Menu: render Trigger directly -> open Menu
end
Menu->>Item: render Menu.Item for each child (clone props, render SidebarLeadingVisual, label)
Item-->>Page: user selects item -> navigation/action
Possibly related issues
Suggested reviewers
🚥 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. 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}
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
…e corresponding CSS styles
paanSinghCoder
changed the title
paanSinghCoder
changed the title
* feat: add accordion and trailingIcon * feat: update Sidebar variant to 'floating' and add accordion group demo with collapsible sections
| align='center' | ||
| gap={3} | ||
| className={cx(styles['nav-leading-icon'], className)} | ||
| aria-hidden='true' |
There was a problem hiding this comment.
can we do a commonProps object and then reuse
There was a problem hiding this comment.
Created a local HOC.
| className, | ||
| label, | ||
| value, | ||
| accordion = false, |
There was a problem hiding this comment.
Let's not call it accordion, we should stick with collapsible
| <span className={cx(styles['nav-leading-icon'], classNames?.icon)}> | ||
| {leadingIcon} | ||
| </span> |
There was a problem hiding this comment.
I think we can reuse the LeadingVisual component here
| <Flex | ||
| direction='column' | ||
| className={cx(styles['nav-group-items'], classNames?.items)} | ||
| role='list' | ||
| > | ||
| {children} | ||
| </Flex> |
There was a problem hiding this comment.
same here too, the icon container styles can be reusued. the hover styles can be passed as classname
| {leadingIcon && ( | ||
| <span | ||
| className={cx(styles['nav-leading-icon'], classNames?.icon)} | ||
| > | ||
| {leadingIcon} | ||
| </span> | ||
| )} |
| {trailingIcon ? ( | ||
| <span | ||
| className={cx( | ||
| styles['nav-group-trailing-icon'], | ||
| classNames?.trailingIcon | ||
| )} | ||
| > | ||
| {trailingIcon} | ||
| </span> | ||
| ) : null} |
| {items.map((item, index) => { | ||
| const { | ||
| leadingIcon: itemLeadingIcon, | ||
| active, | ||
| disabled, | ||
| as = <a />, | ||
| classNames: itemClassNames, | ||
| children: itemLabel, | ||
| ...itemProps | ||
| } = item.props; | ||
|
|
||
| const render = cloneElement( | ||
| as, | ||
| { | ||
| className: cx( | ||
| styles['nav-item'], | ||
| styles['more-menu-item'], | ||
| itemClassNames?.root, | ||
| classNames?.menuItem | ||
| ), | ||
| 'data-active': active, | ||
| 'data-disabled': disabled, | ||
| 'aria-current': active ? 'page' : undefined, | ||
| 'aria-disabled': disabled, | ||
| ...itemProps | ||
| }, | ||
| <> | ||
| <SidebarLeadingVisual | ||
| leadingIcon={itemLeadingIcon} | ||
| fallbackText={ | ||
| typeof itemLabel === 'string' ? itemLabel : undefined | ||
| } | ||
| className={itemClassNames?.leadingIcon} | ||
| /> | ||
| <span | ||
| className={cx( | ||
| styles['nav-text'], | ||
| styles['more-menu-item-text'], | ||
| itemClassNames?.text | ||
| )} | ||
| > | ||
| {itemLabel} | ||
| </span> | ||
| </> | ||
| ); | ||
|
|
There was a problem hiding this comment.
Let's skip this clone element thing and just accept children here.
Let's create a sidebarmorecontext here. And then use that to conditionally render the Sidebar.Item
const insideSidebarMore = useSidebarMoreContext();
if(insideSidebarMore) return <Menu.Item disabled={disabled}/>
else return <Sidebar.Item/>Also I think the Items inside Menu should have the menu item styles instead of the sidebar item styles. Check with design once
| const items = Children.toArray(children).filter( | ||
| isValidElement | ||
| ) as ReactElement<SidebarItemProps>[]; | ||
|
|
||
| if (items.length === 0) return null; |
There was a problem hiding this comment.
This is also not needed
| 'aria-hidden': true | ||
| } as const; | ||
|
|
||
| const content = cloneElement( |
There was a problem hiding this comment.
I think this can be done using useRender instead of clone now
There was a problem hiding this comment.
Using useRender
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/raystack/components/sidebar/sidebar-misc.tsx (2)
158-167: Trailing icon clicks will propagate to accordion trigger.The
trailingIconis rendered inside the accordion header but outside the trigger. However, if the intent is for trailing icon interactions to not toggle the accordion, consider wrapping withonClick={(e) => e.stopPropagation()}on the span, especially if consumers expect action buttons in this slot.🛡️ Proposed fix to prevent event propagation
{trailingIcon ? ( <span className={cx( styles['nav-group-trailing-icon'], classNames?.trailingIcon )} + onClick={(e) => e.stopPropagation()} > {trailingIcon} </span> ) : null}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/sidebar/sidebar-misc.tsx` around lines 158 - 167, The trailing icon span currently rendered for trailingIcon (className using styles['nav-group-trailing-icon'] and classNames?.trailingIcon) can allow clicks to bubble and toggle the accordion; update the span that renders {trailingIcon} to attach an onClick handler that calls e.stopPropagation() (and optionally e.preventDefault() if needed) so interactions with the trailingIcon do not propagate to the accordion trigger.
125-130: Accordion remounts on collapse/expand, resetting user's open/closed state.Using
key={isCollapsed ? 'collapsed' : 'expanded'}forces a full remount of the accordion when the sidebar toggles. This means any user-controlled open/closed state is lost. While this achieves the "force open when collapsed" behavior, it also resets state when expanding back.If preserving user's accordion state across sidebar expand/collapse is desired, consider controlling the accordion's
valueprop based onisCollapsedrather than using a key-based remount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/sidebar/sidebar-misc.tsx` around lines 125 - 130, The Accordion is being force-remounted via key={isCollapsed ? 'collapsed' : 'expanded'}, which resets user open/closed state; remove that key and make AccordionPrimitive.Root controlled instead: replace defaultValue with a controlled value prop (e.g., accordionValue) and add onValueChange to persist user changes; compute the value such that when isCollapsed is true you override to the collapsed-forced value ([groupValue]) but when false you restore the stored accordionValue so the user's previous open/closed state is preserved (initialize accordionValue from groupValue and update it in onValueChange).packages/raystack/components/sidebar/__tests__/sidebar.test.tsx (2)
400-435: Test relies on implicit event behavior that may be fragile.This test wraps
trailingIconwith a<button>that has its ownonClick. The test passes because clicking the inner button doesn't propagate to the accordion trigger. However, from Context Snippet 2 (sidebar-misc.tsx:158-167), thetrailingIconis rendered as a plain<span>without explicitstopPropagation(). If a consumer passes a non-interactive element (e.g., just an icon), clicking it would toggle the accordion.Consider adding a test case with a non-button
trailingIconto document expected behavior, or addstopPropagationin the implementation if accordion should never toggle on trailing icon clicks.🤖 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 400 - 435, The test currently uses a clickable <button> as the trailingIcon which prevents propagation, masking the real behavior; update the Sidebar.Group tests to include a second test that renders a non-interactive trailingIcon (e.g., a plain <span data-testid="group-trailing-icon">+</span>) and assert whether clicking it toggles the accordion to document the intended behavior, or alternatively modify the Sidebar implementation (where trailingIcon is rendered — e.g., the component/function that renders the trailingIcon in Sidebar.Group / sidebar-misc) to call event.stopPropagation() on clicks of the trailing icon so accordion toggling is prevented; pick one approach and add the corresponding test or implementation change and ensure tests cover both interactive (button) and non-interactive trailingIcon cases.
458-469: Test description may be misleading -role='listitem'is always applied.Per Context Snippet 1 (
sidebar-more.tsx:51-72),role='listitem'is applied unconditionally (line 59), not just when collapsed. Onlyaria-labelis conditional onisCollapsed. The test name "sets aria-label for collapsed More trigger" is accurate for thearia-labelassertion, but querying byrole='listitem'doesn't specifically exercise collapsed-only behavior.Consider clarifying the test name or splitting into two assertions: one for the always-present role and one for the conditional aria-label.
🤖 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 458 - 469, The test currently queries the always-present role on Sidebar.More (role='listitem') but claims to verify collapsed-only behavior; update tests around Sidebar.More/isCollapsed: either rename the test to "sets aria-label when collapsed" and keep the aria-label assertion for BasicSidebar open={false}, or split into two tests — one asserting that role='listitem' is always present (render BasicSidebar open={true} and check getByRole('listitem')), and a separate test that renders BasicSidebar open={false} and asserts the conditional aria-label exists on the trigger; reference Sidebar.More and the isCollapsed-dependent aria-label logic in sidebar-more.tsx when making the change.packages/raystack/components/sidebar/sidebar.module.css (1)
274-286: Trailing icon hidden by default, appears on hover.The
opacity: 0with hover reveal is a nice subtle UX pattern. Be aware this may reduce discoverability for new users and is not keyboard-accessible (no:focus-withinrule). Consider adding:focus-withinif the trailing icon contains interactive elements.♻️ Suggested addition for focus accessibility
.nav-group-header:hover .nav-group-trailing-icon { opacity: 1; } + +.nav-group-header:focus-within .nav-group-trailing-icon { + opacity: 1; +}🤖 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 274 - 286, The trailing icon (.nav-group-trailing-icon) is hidden by default via opacity:0 and only shown on .nav-group-header:hover, which is not keyboard-accessible; update the stylesheet to also reveal the icon when the header receives keyboard focus by adding a selector for .nav-group-header:focus-within .nav-group-trailing-icon (and ensure any interactive element inside .nav-group-trailing-icon can receive focus), so keyboard users see the trailing icon the same way hover users do.
🤖 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/sidebar/__tests__/sidebar.test.tsx`:
- Around line 400-435: The test currently uses a clickable <button> as the
trailingIcon which prevents propagation, masking the real behavior; update the
Sidebar.Group tests to include a second test that renders a non-interactive
trailingIcon (e.g., a plain <span data-testid="group-trailing-icon">+</span>)
and assert whether clicking it toggles the accordion to document the intended
behavior, or alternatively modify the Sidebar implementation (where trailingIcon
is rendered — e.g., the component/function that renders the trailingIcon in
Sidebar.Group / sidebar-misc) to call event.stopPropagation() on clicks of the
trailing icon so accordion toggling is prevented; pick one approach and add the
corresponding test or implementation change and ensure tests cover both
interactive (button) and non-interactive trailingIcon cases.
- Around line 458-469: The test currently queries the always-present role on
Sidebar.More (role='listitem') but claims to verify collapsed-only behavior;
update tests around Sidebar.More/isCollapsed: either rename the test to "sets
aria-label when collapsed" and keep the aria-label assertion for BasicSidebar
open={false}, or split into two tests — one asserting that role='listitem' is
always present (render BasicSidebar open={true} and check
getByRole('listitem')), and a separate test that renders BasicSidebar
open={false} and asserts the conditional aria-label exists on the trigger;
reference Sidebar.More and the isCollapsed-dependent aria-label logic in
sidebar-more.tsx when making the change.
In `@packages/raystack/components/sidebar/sidebar-misc.tsx`:
- Around line 158-167: The trailing icon span currently rendered for
trailingIcon (className using styles['nav-group-trailing-icon'] and
classNames?.trailingIcon) can allow clicks to bubble and toggle the accordion;
update the span that renders {trailingIcon} to attach an onClick handler that
calls e.stopPropagation() (and optionally e.preventDefault() if needed) so
interactions with the trailingIcon do not propagate to the accordion trigger.
- Around line 125-130: The Accordion is being force-remounted via
key={isCollapsed ? 'collapsed' : 'expanded'}, which resets user open/closed
state; remove that key and make AccordionPrimitive.Root controlled instead:
replace defaultValue with a controlled value prop (e.g., accordionValue) and add
onValueChange to persist user changes; compute the value such that when
isCollapsed is true you override to the collapsed-forced value ([groupValue])
but when false you restore the stored accordionValue so the user's previous
open/closed state is preserved (initialize accordionValue from groupValue and
update it in onValueChange).
In `@packages/raystack/components/sidebar/sidebar.module.css`:
- Around line 274-286: The trailing icon (.nav-group-trailing-icon) is hidden by
default via opacity:0 and only shown on .nav-group-header:hover, which is not
keyboard-accessible; update the stylesheet to also reveal the icon when the
header receives keyboard focus by adding a selector for
.nav-group-header:focus-within .nav-group-trailing-icon (and ensure any
interactive element inside .nav-group-trailing-icon can receive focus), so
keyboard users see the trailing icon the same way hover users do.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc3bba8f-0ceb-4adf-b99a-bc5159b0673d
📒 Files selected for processing (6)
apps/www/src/app/examples/page.tsxapps/www/src/content/docs/components/sidebar/demo.tsapps/www/src/content/docs/components/sidebar/index.mdxpackages/raystack/components/sidebar/__tests__/sidebar.test.tsxpackages/raystack/components/sidebar/sidebar-misc.tsxpackages/raystack/components/sidebar/sidebar.module.css
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/www/src/content/docs/components/sidebar/demo.ts
- apps/www/src/app/examples/page.tsx
- apps/www/src/content/docs/components/sidebar/index.mdx
2 participants
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]