feat(SideNav): popover for collapsed items with children by cixzhang · Pull Request #680 · facebook/astryx

cixzhang · 2026-03-18T16:48:07Z

Popover for Collapsed SideNav Items with Children

Re-implementation of #655 on current main (post shell hardening batch).

Behavior

When the SideNav is collapsed to icon-only mode, items with sub-items show a popover on hover or click:

Hover: opens after 150ms delay, stays open while hovering popover content, closes 200ms after leaving (same as TopNavMenu)
Click: toggles open/closed
Appears to the end (right in LTR) of the icon-only trigger
Contains children rendered in expanded form via XDSSideNavCollapseProvider
Closes on click outside, Escape, or clicking a child item
Shows the parent label as a header in the popover
Uses design tokens: --color-surface, --color-divider, --elevation-menu, --radius-element

Items without children in collapsed mode: icon-only link/button with tooltip.

Changes

XDSSideNavCollapseContext.ts → .tsx with new XDSSideNavCollapseProvider
XDSSideNavItem: dedicated collapsed render path using useXDSPopover
Hover handlers with delay on both trigger and popover panel
Removed dead isCollapsed references in expanded render path
Uses navItemStyles for all collapsed item styling
8 new tests for collapsed mode

github-actions · 2026-03-18T16:52:31Z

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

Modified Components

SideNav · View in Storybook

Metric	Before	After	Delta
Bundle Size (ESM)	N/A	1.1KB	N/A
Lines of Code	N/A	1489	-
Complexity	N/A	Very High (146)	-

Bundle Size Summary

Package	Size (ESM)	Size (CJS)	Gzipped
@xds/core	10.7KB	17.0KB	2.6KB

Accessibility Audit

Status: 1 accessibility violation(s) found — 1 serious.

XDSSideNav - 1 issue(s)

🟠 serious: Ensure links have discernible text
- Rule: link-name · Affects 1/8 stories · Learn more
- WCAG: 2.4.4, 4.1.2 (Level A)

_{Generated by PR Enrichment workflow | Storybook | Sandbox | View full report}

cixzhang · 2026-03-18T21:23:38Z

  },
+  // Popover surface for collapsed items with children
+  popoverSurface: {
+    backgroundColor: colorVars['--color-surface'],


Should be color-popover

Fixed — updated to colorVars['--color-popover']. ✓

cixzhang · 2026-03-18T21:25:59Z

+              navItemStyles.item,
+              styles.itemCollapsed,
+              isSelected && navItemStyles.selected,
+              isDisabled && navItemStyles.disabled,


xdsClassName('side-nav-item') needs to be alongside these styles since these are the styles that determine the appearance of the nav item.

It might be easier to extract the stylex.props here as the nav item styles since we reapply it in multiple places.

Great call — see commit b7b41b2 which extracts collapsedItemStyles and moves xdsClassName('side-nav-item') to the interactive elements. Much cleaner with the shared variable.

cixzhang · 2026-03-18T21:30:23Z

These also need the xdsClassName('side-nav-item'), class

Fixed in 2c729f3 — added xdsClassName('side-nav-item') to both the <LinkComponent> and <button> in the expanded (non-collapsed) path. They now use mergeProps(xdsClassName('side-nav-item'), stylex.props(...)) matching the collapsed path.

cixzhang

Heads up: the new popoverSurface style uses colorVars["--color-divider"], but PR #698 renames --color-divider → --color-border across the codebase. If #698 merges first, this will need updating to colorVars["--color-border"].

cixzhang · 2026-03-19T00:23:49Z

+    borderStyle: 'solid',
+    borderColor: colorVars['--color-divider'],
+    borderRadius: radiusVars['--radius-2'],
+    boxShadow: elevationVars['--elevation-menu'],


--color-divider is being renamed to --color-border in #698. Will need updating after that PR merges.

Done — updated to colorVars['--color-border'] now that #698 has merged. Also rebased onto main to pick up the rename across the codebase. ✓

When the SideNav is collapsed to icon-only mode, items with sub-items now show a popover on click: - Appears to the end (right in LTR) of the icon-only trigger - Contains children rendered in expanded form (not collapsed) via XDSSideNavCollapseProvider wrapping with isCollapsed: false - Closes on click outside, escape, or clicking a child item - Shows the parent label as a header in the popover - Uses XDS design tokens for surface, border, elevation, and radius Items without children in collapsed mode render as icon-only link/button with tooltip (existing behavior, cleaned up). Changes: - XDSSideNavCollapseContext.ts → .tsx with XDSSideNavCollapseProvider - XDSSideNavItem: collapsed render path with useXDSPopover - Removed dead isCollapsed references in expanded render path - 8 new tests for collapsed mode behavior Co-authored-by: Navi <navi@navibot.dev>

Popover now opens on hover (150ms delay) in addition to click, matching TopNavMenu behavior. Hovering the popover content keeps it open; leaving both trigger and panel closes after 200ms. Co-authored-by: Navi <navi@navibot.dev>

React 19 supports <Context value={...}> without .Provider. Remove the unnecessary Provider wrapper — use XDSSideNavCollapseContext directly in SideNavItem popover and test helpers. CollapseContext file stays as .ts (no JSX needed). Co-authored-by: Navi <navi@navibot.dev>

All useRef/useCallback hooks must run before the early return for items without icons in collapsed mode. React requires the same number of hooks every render. Co-authored-by: Navi <navi@navibot.dev>

Co-authored-by: Navi <navi@navibot.dev>

…ents - Extract shared collapsed item styles (navItemStyles.item + itemCollapsed + selected + disabled) into a single collapsedItemStyles variable - Move xdsClassName('side-nav-item') from wrapper div to the actual item elements (button/link/trigger) where the styles are applied - Wrapper divs now only get styles.root Co-authored-by: Navi <navi@navibot.dev>

- Move xdsClassName('side-nav-item') to expanded link/button elements (was only on wrapper div) - Add marginInlineStart: spacing-1 (4px) to popover surface for gap between trigger and popover Co-authored-by: Navi <navi@navibot.dev>

PR #698 renamed --color-divider to --color-border across the codebase. Updated the popoverSurface borderColor to use the new token name. Co-authored-by: Navi <navi@navibot.dev>

Badge: add label prop (new in #709) to English, Chinese, and dense docs. Section: add xstyle and padding props (xstyle new in #721) to all translations. SideNavItem: add collapsible prop (new in #680) with controlled/uncontrolled mode documentation. Updated nested items example to show collapsible usage. Found during Night Watch doc review. Co-authored-by: Navi <navi@multimango.com>

* feat(SideNav): popover for collapsed items with children When the SideNav is collapsed to icon-only mode, items with sub-items now show a popover on click: - Appears to the end (right in LTR) of the icon-only trigger - Contains children rendered in expanded form (not collapsed) via XDSSideNavCollapseProvider wrapping with isCollapsed: false - Closes on click outside, escape, or clicking a child item - Shows the parent label as a header in the popover - Uses XDS design tokens for surface, border, elevation, and radius Items without children in collapsed mode render as icon-only link/button with tooltip (existing behavior, cleaned up). Changes: - XDSSideNavCollapseContext.ts → .tsx with XDSSideNavCollapseProvider - XDSSideNavItem: collapsed render path with useXDSPopover - Removed dead isCollapsed references in expanded render path - 8 new tests for collapsed mode behavior Co-authored-by: Navi <navi@users.noreply.github.com> * feat: hover to open collapsed subnav popover Popover now opens on hover (150ms delay) in addition to click, matching TopNavMenu behavior. Hovering the popover content keeps it open; leaving both trigger and panel closes after 200ms. Co-authored-by: Navi <navi@users.noreply.github.com> * refactor: remove XDSSideNavCollapseProvider, use context directly React 19 supports <Context value={...}> without .Provider. Remove the unnecessary Provider wrapper — use XDSSideNavCollapseContext directly in SideNavItem popover and test helpers. CollapseContext file stays as .ts (no JSX needed). Co-authored-by: Navi <navi@users.noreply.github.com> * fix: move hooks before early return to prevent hooks violation All useRef/useCallback hooks must run before the early return for items without icons in collapsed mode. React requires the same number of hooks every render. Co-authored-by: Navi <navi@users.noreply.github.com> * fix: use --color-popover for collapsed subnav popover surface Co-authored-by: Navi <navi@users.noreply.github.com> * refactor: extract collapsedItemStyles, move xdsClassName to item elements - Extract shared collapsed item styles (navItemStyles.item + itemCollapsed + selected + disabled) into a single collapsedItemStyles variable - Move xdsClassName('side-nav-item') from wrapper div to the actual item elements (button/link/trigger) where the styles are applied - Wrapper divs now only get styles.root Co-authored-by: Navi <navi@users.noreply.github.com> * fix: add xdsClassName to expanded items, 4px popover offset - Move xdsClassName('side-nav-item') to expanded link/button elements (was only on wrapper div) - Add marginInlineStart: spacing-1 (4px) to popover surface for gap between trigger and popover Co-authored-by: Navi <navi@users.noreply.github.com> * fix: update --color-divider to --color-border after #698 merge PR #698 renamed --color-divider to --color-border across the codebase. Updated the popoverSurface borderColor to use the new token name. Co-authored-by: Navi <navi@users.noreply.github.com> --------- Co-authored-by: Navi <navi@users.noreply.github.com>

* feat(SideNav): popover for collapsed items with children When the SideNav is collapsed to icon-only mode, items with sub-items now show a popover on click: - Appears to the end (right in LTR) of the icon-only trigger - Contains children rendered in expanded form (not collapsed) via XDSSideNavCollapseProvider wrapping with isCollapsed: false - Closes on click outside, escape, or clicking a child item - Shows the parent label as a header in the popover - Uses XDS design tokens for surface, border, elevation, and radius Items without children in collapsed mode render as icon-only link/button with tooltip (existing behavior, cleaned up). Changes: - XDSSideNavCollapseContext.ts → .tsx with XDSSideNavCollapseProvider - XDSSideNavItem: collapsed render path with useXDSPopover - Removed dead isCollapsed references in expanded render path - 8 new tests for collapsed mode behavior * feat: hover to open collapsed subnav popover Popover now opens on hover (150ms delay) in addition to click, matching TopNavMenu behavior. Hovering the popover content keeps it open; leaving both trigger and panel closes after 200ms. * refactor: remove XDSSideNavCollapseProvider, use context directly React 19 supports <Context value={...}> without .Provider. Remove the unnecessary Provider wrapper — use XDSSideNavCollapseContext directly in SideNavItem popover and test helpers. CollapseContext file stays as .ts (no JSX needed). * fix: move hooks before early return to prevent hooks violation All useRef/useCallback hooks must run before the early return for items without icons in collapsed mode. React requires the same number of hooks every render. * fix: use --color-popover for collapsed subnav popover surface * refactor: extract collapsedItemStyles, move xdsClassName to item elements - Extract shared collapsed item styles (navItemStyles.item + itemCollapsed + selected + disabled) into a single collapsedItemStyles variable - Move xdsClassName('side-nav-item') from wrapper div to the actual item elements (button/link/trigger) where the styles are applied - Wrapper divs now only get styles.root * fix: add xdsClassName to expanded items, 4px popover offset - Move xdsClassName('side-nav-item') to expanded link/button elements (was only on wrapper div) - Add marginInlineStart: spacing-1 (4px) to popover surface for gap between trigger and popover * fix: update --color-divider to --color-border after #698 merge PR #698 renamed --color-divider to --color-border across the codebase. Updated the popoverSurface borderColor to use the new token name. ---------

@kentonquatman

The collapsed submenu flyout painted two surfaces. `usePopover` already paints the panel it renders into — `--color-background-popover`, a `--radius-container` corner and `--shadow-low` — and the content div underneath it added a 1px `--color-border` rectangle at radius 0, so the flyout showed square grey corners inside rounded ones. Every other popover consumer in core (DropdownMenu, TopNavMenu, Selector, TabMenu) leaves the chrome to the surface; SideNavItem and TopNavMegaMenu were the only two adding a border of their own. The same div carried `marginInlineStart: --spacing-1`. Inside the panel that is not a gap from the rail — it insets the content, leaving an unbalanced 4px strip down the flyout's inside edge with the painted panel poking out past it. The gap belongs on the positioned layer, which is where DropdownMenu puts its `popoverInlineGap`, so it moves there and becomes what it was always meant to be: 4px between the trigger and the flyout. The content lands on exactly the same pixel column as before (x=470 at a 900px viewport); what moves is the painted panel's left edge, from 4px behind the content to flush with it. Reported on #4880 by @kentonquatman. Pre-existing since #680 — the border predates `usePopover` gaining a default surface. Verified: 224 tests green across SideNav and TopNav; flyout geometry measured in real Chromium before and after (border 1px→0, content inset 4/0 → 0/0, panel radius 12px with no square inner rectangle).

@kentonquatman

The collapsed submenu flyout painted two surfaces. `usePopover` already paints the panel it renders into — `--color-background-popover`, a `--radius-container` corner and `--shadow-low` — and the content div underneath it added a 1px `--color-border` rectangle at radius 0, so the flyout showed square grey corners inside rounded ones. Every other popover consumer in core (DropdownMenu, TopNavMenu, Selector, TabMenu) leaves the chrome to the surface; SideNavItem and TopNavMegaMenu were the only two adding a border of their own. The same div carried `marginInlineStart: --spacing-1`. Inside the panel that is not a gap from the rail — it insets the content, leaving an unbalanced 4px strip down the flyout's inside edge with the painted panel poking out past it. The gap belongs on the positioned layer, which is where DropdownMenu puts its `popoverInlineGap`, so it moves there and becomes what it was always meant to be: 4px between the trigger and the flyout. The content lands on exactly the same pixel column as before (x=470 at a 900px viewport); what moves is the painted panel's left edge, from 4px behind the content to flush with it. Reported on #4880 by @kentonquatman. Pre-existing since #680 — the border predates `usePopover` gaining a default surface. Verified: 224 tests green across SideNav and TopNav; flyout geometry measured in real Chromium before and after (border 1px→0, content inset 4/0 → 0/0, panel radius 12px with no square inner rectangle).

@kentonquatman

The collapsed submenu flyout painted two surfaces. `usePopover` already paints the panel it renders into — `--color-background-popover`, a `--radius-container` corner and `--shadow-low` — and the content div underneath it added a 1px `--color-border` rectangle at radius 0, so the flyout showed square grey corners inside rounded ones. Every other popover consumer in core (DropdownMenu, TopNavMenu, Selector, TabMenu) leaves the chrome to the surface; SideNavItem and TopNavMegaMenu were the only two adding a border of their own. The same div carried `marginInlineStart: --spacing-1`. Inside the panel that is not a gap from the rail — it insets the content, leaving an unbalanced 4px strip down the flyout's inside edge with the painted panel poking out past it. The gap belongs on the positioned layer, which is where DropdownMenu puts its `popoverInlineGap`, so it moves there and becomes what it was always meant to be: 4px between the trigger and the flyout. The content lands on exactly the same pixel column as before (x=470 at a 900px viewport); what moves is the painted panel's left edge, from 4px behind the content to flush with it. Reported on #4880 by @kentonquatman. Pre-existing since #680 — the border predates `usePopover` gaining a default surface. Verified: 224 tests green across SideNav and TopNav; flyout geometry measured in real Chromium before and after (border 1px→0, content inset 4/0 → 0/0, panel radius 12px with no square inner rectangle).

meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Mar 18, 2026

cixzhang force-pushed the navi/feat/collapsed-subnav-popover branch from bad015f to cf73d29 Compare March 18, 2026 21:02

cixzhang mentioned this pull request Mar 18, 2026

Deep review: Shell & Navigation surface #565

Closed

55 tasks

cixzhang marked this pull request as ready for review March 18, 2026 21:22

cixzhang commented Mar 18, 2026

View reviewed changes

cixzhang force-pushed the navi/feat/collapsed-subnav-popover branch from a0e10fa to 2c729f3 Compare March 18, 2026 22:27

cixzhang commented Mar 19, 2026

View reviewed changes

cixzhang force-pushed the navi/feat/collapsed-subnav-popover branch 2 times, most recently from bc9ed49 to 5e29dab Compare March 19, 2026 02:51

cixzhang and others added 8 commits March 19, 2026 16:43

feat: hover to open collapsed subnav popover

8b03c08

Popover now opens on hover (150ms delay) in addition to click, matching TopNavMenu behavior. Hovering the popover content keeps it open; leaving both trigger and panel closes after 200ms. Co-authored-by: Navi <navi@navibot.dev>

fix: move hooks before early return to prevent hooks violation

2c2cd05

All useRef/useCallback hooks must run before the early return for items without icons in collapsed mode. React requires the same number of hooks every render. Co-authored-by: Navi <navi@navibot.dev>

fix: use --color-popover for collapsed subnav popover surface

1ad646a

Co-authored-by: Navi <navi@navibot.dev>

fix: update --color-divider to --color-border after #698 merge

201a3f4

PR #698 renamed --color-divider to --color-border across the codebase. Updated the popoverSurface borderColor to use the new token name. Co-authored-by: Navi <navi@navibot.dev>

cixzhang force-pushed the navi/feat/collapsed-subnav-popover branch from 5e29dab to 201a3f4 Compare March 19, 2026 16:50

cixzhang merged commit f33a92c into main Mar 20, 2026
11 checks passed

cixzhang mentioned this pull request Mar 20, 2026

Hardening: Cindy's Components (31) #719

Closed

josephfarina mentioned this pull request Mar 20, 2026

docs: add missing props for Badge, Section, SideNavItem #743

Merged

4 tasks

cixzhang deleted the navi/feat/collapsed-subnav-popover branch March 20, 2026 16:58

Navigation Menu

Uh oh!

feat(SideNav): popover for collapsed items with children - #680

feat(SideNav): popover for collapsed items with children#680
cixzhang merged 8 commits into
mainfrom
navi/feat/collapsed-subnav-popover

cixzhang commented Mar 18, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Mar 18, 2026 •

edited

Loading

Uh oh!

cixzhang Mar 18, 2026

Uh oh!

cixzhang Mar 18, 2026

Uh oh!

cixzhang Mar 18, 2026

Uh oh!

cixzhang Mar 18, 2026

Uh oh!

cixzhang Mar 18, 2026

Uh oh!

cixzhang Mar 18, 2026

Uh oh!

cixzhang left a comment

Uh oh!

cixzhang Mar 19, 2026

Uh oh!

cixzhang Mar 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

cixzhang commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Popover for Collapsed SideNav Items with Children

Behavior

Changes

Uh oh!

github-actions Bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Analysis Report

📚 Storybook Preview

🧪 Sandbox Preview

Modified Components

Bundle Size Summary

Accessibility Audit

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cixzhang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cixzhang commented Mar 18, 2026 •

edited

Loading

github-actions Bot commented Mar 18, 2026 •

edited

Loading