CORE-2005: Migrate navigation components (SidebarNav, ButtonNav, Pagination) to plain CSS

[OpenStaxClaude]

Jira: CORE-2005 (parent: CORE-1777 — move ui-components off styled-components)

Follows the pattern established by CORE-2004 / #130.

Stacked on #143 (CORE-2720-global-css-theme-tokens). Review that one first; this diff is only the navigation components. Not just bookkeeping — #143's tokens.spec.ts fails on all three of the CSS files added here unless they use the global tokens, so the two PRs are incompatible in whichever order they land.

What changed

Component	Before	After
SidebarNav	SidebarNav/styles.ts (navStyles + 4 styled sub-components)	SidebarNav/SidebarNav.css
ButtonNav	ButtonNav/styles.ts (4 styled components)	ButtonNav/ButtonNav.css
Pagination	two inline styled(...) wrappers in index.tsx	Pagination/Pagination.css

Theme values come from the global :root tokens added in #143 — each component's CSS reads var(--component-hook, var(--ox-token)), so the --sidebar-nav-* / --pagination-* / --button-nav-* override hooks stay public while the defaults track the theme. classNames handles conditional classes.

collapsedWidth / expandedWidth moved to a new SidebarNav/constants.ts so SidebarNavStyles keeps exporting them — assignments reads SidebarNavStyles.collapsedWidth in three places.

Notes for review

Sidebar backdrop z-indexes are literals, not variables. Resolved by stacking on #143. The rules .sidebar-nav.mobile ~ main::before and ~ [data-backdrop-target]::before style siblings of the nav, and component-scoped custom properties inherit down the tree rather than across siblings — so while theme values were bound inline on the nav, those two z-indexes had to be written as the literals 11 and 19. The :root tokens reach siblings fine, so they are now calc(var(--ox-z-index-navbar) + 1) and calc(var(--ox-z-index-sidebar) - 1). margin-left: 5.6rem on .mobile + nav is unchanged; it is a layout value, not a theme one.

--collapsed-width / --expanded-width stay declared in the CSS file rather than bound inline, because @media (max-width: 15em) reassigns --expanded-width to 100vw and an inline binding would always beat the media query.

One hook-ordering fix. Pagination called usePaginationRanges after an early return null for totalPages <= 1. The styled() wrapper meant eslint didn't see it as a component, so react-hooks/rules-of-hooks never fired; as a plain component it does.

Per review, this is now fixed structurally rather than by hoisting the hook: the exported Pagination is a gatekeeper that only decides whether there is anything to paginate, and the page list lives in PaginationPages, which it renders when there is. The hooks sit in PaginationPages and are unconditional by construction, so nothing relies on a comment telling the next person not to add a hook above an early return. usePaginationRanges also no longer runs at all for a single page (verified: 0 calls at totalPages=1, 1 at totalPages=10).

The guard widened from totalPages === 0 || totalPages === 1 to totalPages > 1, so a negative count renders nothing rather than a nav built from a negative range. There is a test for it.

This is a real bug — React throws "rendered fewer hooks than expected" when totalPages changes to 1. Correction: it was not. Writing the regression test Copilot asked for showed the old ordering passes. React's check is currentHook !== null && currentHook.next !== null, so it only fires once at least one hook has run on the update; usePaginationRanges was the component's only hook, so the short render consumed zero hooks and the mismatch went undetected. The old ordering was a latent landmine — the next hook added above that early return would have turned it into a genuine crash — not a live one.

Pagination.css keys off li > a rather than .pagination-link. Eight downstream repos wrap UI.LinkForPage in their own Page component and assessments' Review screen renders it directly; keying off the element keeps them styled even if a consumer supplies its own anchor. LinkForPage still carries a pagination-link class for anyone who wants to target it.

Backward compatibility

Public React APIs and rendered DOM are unchanged apart from class names — the regenerated snapshots differ only in class/style attributes, structure is identical. styled(SidebarNav) and styled(BodyPortalSidebarNav) still work (assignments uses both) because className is still forwarded.

What no longer works is interpolating a sub-component as a selector, e.g. ${SidebarNavStyles.ToggleButton} { ... }. An org-wide code search finds that only in this repo's own SidebarNav.stories.tsx, updated here to use .sidebar-nav-toggle.

Worth a smoke test in assignments against a pre-release tag before release: consumer styled() overrides now compete with a plain .sidebar-nav class at similar specificity rather than with a single generated class, so injection order matters more than it did. This is the same trade-off the earlier phases of CORE-1777 accepted.

Verification

• npm test — 386 passing, 40 suites, no failures. The ProfileMenu react-aria snapshot failure noted here through earlier rounds is gone: CORE-2715: replace flaky ProfileMenu snapshot with targeted assertions #138 landed on main and replaced that snapshot with targeted assertions, so it is no longer in the stack.
• npm run lint, npm run typecheck — clean.
• npm run build — succeeds; confirmed all three new .css files are rsynced into dist/esm and dist/cjs.
• Ladle visual verification done. Captured 9 states with Playwright against a Ladle dev server, on this branch and on main, then pixel-diffed them:

State	Diff vs main
SidebarNav desktop expanded / collapsed	identical
SidebarNav mobile collapsed / expanded-with-backdrop	identical
SidebarNav BodyPortal desktop	0.06% — the story's own Date.now() text
SidebarNav BodyPortal mobile backdrop	0.15% — same Date.now() text
ButtonNav default (prev arrow disabled)	identical
ButtonNav with toggle groups	identical
Pagination (all 8 examples)	identical

Seven of nine are pixel-for-pixel identical; the two that differ do so only in the bounding box of the timestamp the UsingBodyPortal story renders itself. Specifically confirmed by eye: the mobile backdrop still dims main while leaving the expanded sidebar above it (the sibling-selector rule with the hardcoded z-indexes), and the disabled ButtonNav arrow still renders at 0.5 opacity via :disabled rather than the old prop interpolation.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

[RoyEJohnson]

Shall we stack this PR on ui-components/pull/143? Seems like all the open PRs should, actually. What do you think?

[OpenStaxClaude]

@RoyEJohnson Yes — done, this one is now based on CORE-2720-global-css-theme-tokens. It turned out to be more than a tidiness call: the two PRs are incompatible in whichever order they land, so one of us was always going to do this work.

Why it wasn't optional

Rebasing onto #143 and running its own checker:

● component CSS › components/SidebarNav/SidebarNav.css uses tokens rather than repeating theme values
  + "#f5f5f5 duplicates the theme — use var(--ox-color-neutral-bright)"
  + "#6f6f6f duplicates the theme — use var(--ox-color-neutral-thin)"
  + "#fff duplicates the theme — use var(--ox-color-white)"
  + "#959595 is not a theme value — add it to palette.ts, or to KNOWN_OFF_PALETTE ..."

...and the same for Pagination.css and ButtonNav.css — eight violations across the three files. tokens.spec.ts actively rejects the pattern this PR was landing. Merging #140 to main first would just have meant extending #143's sweep to cover it afterwards.

So the three components now follow your convention: var(--component-hook, var(--ox-token)) in the CSS, import '../../theme/theme.css', and the inline CSSPropertiesWithVariables objects deleted.

The nice part: it kills a caveat I'd written into the description

The PR previously said the sidebar backdrop z-indexes had to be literals. The reason was real — .sidebar-nav.mobile ~ main::before styles a sibling of the nav, and component-scoped custom properties inherit down the tree, not across siblings, so a value bound inline on the nav never reached the backdrop. :root tokens have no such problem:

z-index: calc(var(--ox-z-index-navbar) + 1);   /* was: 11 */
z-index: calc(var(--ox-z-index-sidebar) - 1);  /* was: 19 */

That's two hardcoded numbers that were silently coupled to src/theme.ts and are now actually coupled to it.

Two judgement calls to check

#959595 (toggle button border) is genuinely off-palette — I carried it over verbatim from styles.ts during the migration. Per the policy documented on KNOWN_OFF_PALETTE I added it there with a reason rather than snapping it to neutralMedium (#a0a0a0), since that would be a visual change rather than a refactor. Say the word if you'd rather take the visual change. #fff next to it was a theme value and is now var(--ox-color-white).

Three tests deleted. The binds the theme values as custom properties tests I added last round asserted the inline bindings, which no longer exist — and tokens.spec.ts now covers that wiring centrally and better. I kept a test where a public contract survives: that consumers can still override through the documented --sidebar-nav-* / --pagination-* hooks.

On "all the open PRs should" — mostly, but not #138 and #139

I checked the file overlaps, and for those two the dependency runs the other way:

PR	Overlap with #143
#140 (this)	tokens.spec.ts only	stacked ✅
#137	NavBarMenuButtons.tsx, Tree/TreeCheckbox.tsx	genuinely overlaps — worth stacking one way or the other
#141	—	already stacked on #137
#138	ProfileMenu/…/index.spec.tsx.snap	#143 edits the snapshot #138 deletes
#139	HelpMenu/…/index.spec.tsx.snap	#143 edits the snapshot #139 deletes
#125	—	not ours

#143 had to regenerate the ProfileMenu and HelpMenu snapshots because the token sweep changed values inside them. #138 and #139 delete those snapshot files outright. So if they land first, #143 doesn't need to touch either file and that part of its diff disappears; stacking them on #143 instead means #143 does churn that's immediately thrown away.

There's a practical reason to prefer that order too: the flaky ProfileMenu snapshot #138 removes is currently failing in a full-suite run on #143's branch — I confirmed it fails identically there with a cold cache, without any of my commits. Landing #138 early clears it.

For #137 the overlap is real code, so it and #143 should be sequenced — I'd suggest #143 rebasing onto #137, since #137 already has #141 stacked on it and is much the smaller diff, but you know the merge order you want better than I do.

Verification

• Full suite on this branch, cold cache: 370 passed, 1 failed — the ProfileMenu snapshot above, which fails identically on the base branch without my commits (352 passed / 1 failed there). Not from this PR.
• tokens.spec.ts: 94 passed, including the three new CSS files.
• lint + typecheck clean; CI green.
• Pre-rebase SHA was 104f0054 if you'd rather I unwind the stack.

[OpenStaxClaude]

@RoyEJohnson Resolved — but the conflicts weren't in this PR, so flagging where they actually were.

#140 merge-tested clean against its base the whole time (GitHub had it MERGEABLE/CLEAN). The blocker was one level down: #143 was CONFLICTING against #137, and since #140 sits on top of #143, nothing here could land. Fixed #143, then followed with this one.

What was wrong with #143

Its branch still carried a stale copy of #137's commit (590bda0d) from before #137 was force-pushed onto a newer main. That newer main includes #138 and #139, which landed on 9/1 and delete the ProfileMenu and HelpMenu snapshots — the exact files #143's token sweep was editing. Hence the modify/delete conflicts:

src/components/ProfileMenu/__snapshots__/index.spec.tsx.snap
src/components/NavBarMenuButtons.tsx
src/components/NavBarMenuButtons.spec.tsx
src/components/Tree/TreeCheckbox.tsx

Rebased #143's own ten commits onto the current #137 head, dropping the stale duplicate. Two resolutions, both the same underlying call:

• ProfileMenu snapshot — took the deletion. CORE-2715: replace flaky ProfileMenu snapshot with targeted assertions #138 replaced it with targeted assertions; I confirmed ProfileMenu/index.spec.tsx has zero toMatchSnapshot calls on the new base, so re-adding the file would have resurrected a snapshot nothing asserts.
• 542b66e3 "Restore react-aria focus attributes in the ProfileMenu snapshot" — dropped entirely. Checked it first: one file, two lines, all in that snapshot. It existed only to patch the file CORE-2715: replace flaky ProfileMenu snapshot with targeted assertions #138 deleted, so it's dead.

The NavBarMenuButtons / TreeCheckbox conflicts never materialised in the rebase — f9cf8331 "reconcile the CORE-2710 style tests with the token sweep" already handles that interplay, and replaying it onto the real #137 applied cleanly.

7f8bdfb → f9cf8331, 10 commits → 9.

Then this PR

Rebased onto the new #143 head, f1f4ad9 → 2e5825ae. All six commits replayed with no conflicts, your 48db107e comment fix included. This PR's own diff is unchanged — still 17 files, +747/−1152, same as before the move.

Stack now reads main → #137 → #143 → #140, and both #143 and #140 report MERGEABLE/CLEAN with green checks.

One nice side effect

The full suite is now 386 passing, 40 suites, zero failures. The ProfileMenu react-aria snapshot failure I'd been reporting as pre-existing for several rounds is genuinely gone — #138 was the fix for it, and it's in the base now. I've updated the Verification section of the description, which still carried the old caveat.

Recovery points if you want any of this unwound: #143 was 7f8bdfb36, #140 was f1f4ad920.