fix(frontend): show submenu flyout and tooltip links when sidebar is collapsed (EVO-1048)#54
fix(frontend): show submenu flyout and tooltip links when sidebar is collapsed (EVO-1048)#54marcelogorutuba wants to merge 3 commits into
Conversation
…collapsed (EVO-1048) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reviewer's GuideThis PR updates the sidebar layout so that submenu panels and tooltip content work correctly when the sidebar is collapsed, including a flyout-style secondary panel with backdrop and richer, clickable submenu tooltips. Sequence diagram for collapsed sidebar submenu flyout behaviorsequenceDiagram
actor User
participant Sidebar
participant MainLayout
User->>Sidebar: clickMenuItem
Sidebar->>Sidebar: setActiveSubmenu
Sidebar->>MainLayout: render with isCollapsed true
MainLayout->>Sidebar: render submenu panel
Sidebar->>Sidebar: [activeSubmenu && isCollapsed]
Sidebar->>Sidebar: renderBackdrop
Sidebar->>Sidebar: renderSubmenuPanel absolute left_16 top_0
User->>Sidebar: clickBackdrop
Sidebar->>Sidebar: setActiveSubmenu null
Sidebar->>MainLayout: rerender without submenu panel
Sequence diagram for tooltip submenu links in MenuItemsequenceDiagram
actor User
participant MenuItem
participant TooltipContent
participant LinkComponent
User->>MenuItem: hover
MenuItem->>TooltipContent: render side right
MenuItem->>TooltipContent: [hasSubItems]
TooltipContent->>TooltipContent: renderSubItemHeader item.name
TooltipContent->>TooltipContent: map item.subItems
TooltipContent->>LinkComponent: render to subItem.href
User->>LinkComponent: click
LinkComponent-->>User: navigate to subItem.href
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The submenu tooltip now assumes every subItem has an icon (
<subItem.icon />); if any menu configs omit an icon this will throw at runtime, so consider rendering the icon conditionally or providing a fallback. - The collapsed-mode backdrop is
absolute inset-0 z-40, so it will cover the entire positioned ancestor; double-check that the ancestor is intentionallyrelativeand, if not, consider constraining the backdrop to just the sidebar/flyout area to avoid intercepting unintended clicks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The submenu tooltip now assumes every subItem has an icon (`<subItem.icon />`); if any menu configs omit an icon this will throw at runtime, so consider rendering the icon conditionally or providing a fallback.
- The collapsed-mode backdrop is `absolute inset-0 z-40`, so it will cover the entire positioned ancestor; double-check that the ancestor is intentionally `relative` and, if not, consider constraining the backdrop to just the sidebar/flyout area to avoid intercepting unintended clicks.
## Individual Comments
### Comment 1
<location path="src/components/layout/components/MenuItem.tsx" line_range="83" />
<code_context>
+ to={subItem.href}
+ className="flex items-center gap-2.5 px-3 py-2 text-sm text-primary-foreground hover:bg-primary-foreground/15 transition-colors"
+ >
+ <subItem.icon className="h-4 w-4 flex-shrink-0 text-primary-foreground/70" />
+ <span>{subItem.name}</span>
+ </Link>
</code_context>
<issue_to_address>
**suggestion:** Consider explicitly marking the icon as decorative for accessibility if it doesn't convey unique information.
If the icon is decorative and the text already conveys the meaning, ensure the icon component either sets `aria-hidden="true"` or supports a prop to do so to prevent redundant screen reader announcements.
```suggestion
<subItem.icon
className="h-4 w-4 flex-shrink-0 text-primary-foreground/70"
aria-hidden="true"
/>
```
</issue_to_address>
### Comment 2
<location path="src/components/layout/components/Sidebar.tsx" line_range="123-129" />
<code_context>
{/* Second Sidebar for Submenus */}
- {activeSubmenu && !isCollapsed && (
- <div className="hidden md:flex w-64 bg-sidebar text-sidebar-foreground flex-col border-r border-sidebar-border">
+ {activeSubmenu && isCollapsed && (
+ <div
+ className="hidden md:block absolute inset-0 z-40"
+ onClick={() => setActiveSubmenu(null)}
+ />
+ )}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider keyboard accessibility for the overlay used to dismiss the submenu.
Currently the overlay is a non-interactive `div` with only an `onClick` handler, so keyboard users can’t dismiss the submenu. Either make it an interactive element (e.g. add `role="button"` and a key handler for Enter/Escape) or hook into an existing focus/blur mechanism to close the submenu without relying solely on a clickable div.
```suggestion
{/* Second Sidebar for Submenus */}
{activeSubmenu && isCollapsed && (
<div
role="button"
tabIndex={0}
aria-label="Close submenu"
className="hidden md:block absolute inset-0 z-40"
onClick={() => setActiveSubmenu(null)}
onKeyDown={(event) => {
if (event.key === 'Enter' || event.key === ' ' || event.key === 'Escape') {
event.preventDefault();
setActiveSubmenu(null);
}
}}
/>
)}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| to={subItem.href} | ||
| className="flex items-center gap-2.5 px-3 py-2 text-sm text-primary-foreground hover:bg-primary-foreground/15 transition-colors" | ||
| > | ||
| <subItem.icon className="h-4 w-4 flex-shrink-0 text-primary-foreground/70" /> |
There was a problem hiding this comment.
suggestion: Consider explicitly marking the icon as decorative for accessibility if it doesn't convey unique information.
If the icon is decorative and the text already conveys the meaning, ensure the icon component either sets aria-hidden="true" or supports a prop to do so to prevent redundant screen reader announcements.
| <subItem.icon className="h-4 w-4 flex-shrink-0 text-primary-foreground/70" /> | |
| <subItem.icon | |
| className="h-4 w-4 flex-shrink-0 text-primary-foreground/70" | |
| aria-hidden="true" | |
| /> |
🔥 Code Review — Follow-ups (AI Review)Findings from adversarial review against EVO-1048 acceptance criteria. AC coverage: PARTIAL (hover UI diverges from click UI), no regressions in expanded mode, all submenu entries ( 🔴 High
🟡 Medium
🟢 Low
🤖 Generated by |
…debar flyout - Simplify collapsed Tooltip to show item name only (remove interactive links anti-pattern) - Fix backdrop to start at left-16 so icon sidebar remains directly clickable (no 2-click toggle) - Move backdrop from Sidebar into MainLayout to decouple overlay from sidebar component - Add Escape key dismissal for collapsed flyout (WAI-ARIA) - Add focus management: move focus into flyout on open, restore to trigger on close - Add transition animation (duration-150) to flyout open/close - Add Sidebar.spec.tsx covering collapsed+activeSubmenu scenarios (7 tests) - Replace local cn helpers with shared import from @/utils/cn - Add comment on relative container in MainLayout explaining flyout anchor role Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…psed flyout - Fix animation: keep flyout container always mounted when collapsed so CSS opacity+translate transitions actually run on show/hide - Fix accessible name: add aria-label to close button - Fix focus management: only capture previousFocusRef on first open, not when switching between submenus - Add keyboard trap: Tab cycles within flyout and cannot escape - Extract submenuHeader/submenuItems helpers to remove JSX duplication - Update Sidebar.spec.tsx: fresh vi.fn() per test via makeProps factory, add tests for animation classes, aria-label, and Tab trap (11 tests) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔥 Code Review — Follow-ups (AI Review, iter 2)Adversarial review against EVO-1048 acceptance criteria. AC coverage: ✅ AC #1 (click opens flyout when collapsed), ✅ AC #2 (every entry with submenu covered by the same code path), ✅ AC #3 (no regression in expanded mode — inline flyout preserved via class conditional). Status of prior iteration: 9/9 prior findings addressed — simplified tooltip, backdrop fixed with Now, new findings on the latest commit: 🔴 High
🟡 Medium
🟢 Low
🤖 Generated by |
2 participants
Summary
Tooltip— shows item name only (hover informational, click opens flyout)left-16inMainLayoutso icon sidebar stays directly clickable without 2-click toggleopacity + translate-xtransitions to work correctlyaria-labelto close button (lucide X icon isaria-hidden; screen readers had no accessible name)Tabcycles within flyout;Shift+Tabwraps backward; cannot escape to main contentSidebarintoMainLayoutto decouple overlay from sidebar componentsubmenuHeader/submenuItemshelpers — eliminates JSX duplication between collapsed and expanded branchescnhelpers with shared@/utils/cnrelativecontainer inMainLayoutexplaining flyout anchor roleValidation
pnpm exec tsc -b --noEmit✅ (no errors in layout files)pnpm exec eslint src/components/layout/MainLayout.tsx src/components/layout/components/MenuItem.tsx src/components/layout/components/Sidebar.tsx✅pnpm test src/components/layout/components/Sidebar.spec.tsx→ 11/11 ✅Changed Files
src/components/layout/MainLayout.tsxsrc/components/layout/components/Sidebar.tsxsrc/components/layout/components/MenuItem.tsxsrc/components/layout/components/Sidebar.spec.tsxLinked Issue