Skip to content

CORE-2006: Migrate ProfileMenu and HelpMenu components to plain CSS - #141

Open
OpenStaxClaude wants to merge 4 commits into
CORE-2720-global-css-theme-tokensfrom
CORE-2006-migrate-profile-and-help-menus
Open

CORE-2006: Migrate ProfileMenu and HelpMenu components to plain CSS#141
OpenStaxClaude wants to merge 4 commits into
CORE-2720-global-css-theme-tokensfrom
CORE-2006-migrate-profile-and-help-menus

Conversation

@OpenStaxClaude

@OpenStaxClaude OpenStaxClaude commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Jira: CORE-2006 (subtask of CORE-1777)

Migrates ProfileMenu/index.tsx and HelpMenu/index.tsx off styled-components, following the same hybrid pattern as the already-merged subtasks (#130 NavBar, #131 ManageCookies): theme values stay in JavaScript and are bound to CSS custom properties on the element, with the palette hex inlined as the CSS fallback.

Review round 6 — history linearised onto the current #143 head

The branch had picked up a merge commit: #143 rebased (onto main, so it now carries #138 and #137), and its new head was merged into this branch rather than this branch being rebased onto it. That left both the old and the new copies of #143's commits in the history. Reset to #143's current head and replayed only this PR's four commits, so the history is linear again and the diff is exactly the six files below. The #138 cherry-pick this PR used to carry is gone too — #143 contains #138 now, so it is redundant.

On the base itself — it is CORE-2720-global-css-theme-tokens because of the round-5 request to stack on #143 and use the global tokens, and it stays there unless you say otherwise. Flipping to main is one command on my side, but it would mean dropping the token-sweep commit and going back to palette hexes as the var() fallbacks until #143 lands, since theme.css and the --ox-* tokens do not exist on main. That shape was green two rounds ago, so it is a cheap revert — just say which you'd rather have.

Review round 5 — stacked on #143 and moved onto the global tokens

Done as asked: base is CORE-2720-global-css-theme-tokens, and the two new stylesheets read --ox-* tokens instead of repeating palette hexes — var(--help-menu-button-color, var(--ox-color-gray)) rather than var(--help-menu-button-color, #5e5e5e). Eleven fallbacks in total. src/theme/tokens.spec.ts passes on both files, so they're now covered by the same guard as the rest of the library.

The override hooks are unchanged; only their defaults moved from JavaScript to CSS, exactly as #143 did elsewhere. So these components no longer bind static custom properties inline, style is no longer destructured in any of the four wrappers, and it passes straight through in ...props.

That removes the #137 dependency rather than deferring it. The bug CORE-2710 guards against is a wrapper overwriting the caller's style — which these wrappers no longer do. ProfileMenuItem/HelpMenuItem need nothing from #137 now, and the composeRenderProps wrapping I'd added for the two buttons is gone with the defaults it existed to protect. Same reasoning as the note #143 leaves on NavBarMenuItem. className composition stays: that one is a real bug fix, not a default.

iframeWrapperStyle and putAwayStyle are gone too — the contact-form iframe wrapper and the put-away bar take their colours from HelpMenu.css.

One thing carried deliberately. This base predates #138, which is already on main. Without it the old flaky ProfileMenu snapshot fails against the migrated component, so #138 is cherry-picked here (c7ff9ac, authorship preserved) and shows in the diff below as the snapshot deletion plus the targeted-assertion spec. It drops out as a duplicate as soon as #143 rebases onto main. Happy to drop it sooner if you'd rather rebase #143 first.

Changes

ProfileMenu — new ProfileMenu/ProfileMenu.css

  • ProfileMenuButton (styled(Button)) → forwardRef wrapper over react-aria Button, class profile-menu-button. Colours are var(--profile-menu-button-color, var(--ox-color-white)) / var(--profile-menu-button-bg, var(--ox-color-dark-teal)) in the CSS — the hooks are still there for consumers, the defaults just aren't set inline. The defaultFocusOutline fragment is inlined under [data-focused], [data-focus-visible], matching how NavBarMenuButtons.css handles it.
  • ProfileMenuPopover (styled(NavBarPopover)) → forwardRef wrapper, class profile-menu-popover.
  • ProfileMenuItem (styled(NavBarMenuItem)) → forwardRef wrapper, class profile-menu-item; var(--profile-menu-item-color, var(--ox-color-neutral-darker)) in the CSS.

HelpMenu — new HelpMenu/HelpMenu.css

  • HelpMenuButton (styled(NavBarMenuButton)) → function component, class help-menu-button; var(--help-menu-button-color, var(--ox-color-gray)) in the CSS.
  • HelpMenuItem (styled(NavBarMenuItem)) → forwardRef wrapper, class help-menu-item; var(--help-menu-item-color, var(--ox-color-neutral-darker)) and var(--help-menu-item-focus-bg, var(--ox-color-neutral-lighter)) in the CSS.
  • IframeWrapper (styled(BodyPortal)) and Iframe (styled.iframe) → BodyPortal + plain iframe with help-menu-iframe-wrapper / help-menu-iframe; their colours come from the stylesheet, so no inline style is passed at all.
  • StyledPutAway (styled(PutAway)) → the internal PutAway now carries help-menu-put-away directly; the @media(min-width: 56em) block is a standard media query. PutAway keeps its className prop.

NewTabIcon is untouched — its fill={colors.palette.neutralThin} was already plain JS, not styled-components.

Public API is unchanged: every exported name still exists, still accepts className / style (so a consumer's styled(...) extension keeps working), and ProfileMenuButton / ProfileMenuItem / HelpMenuItem forward refs exactly as the styled versions did.

⚠️ This restores a popover position that #130 shifted by 14px — please confirm you want it

While screenshotting the Ladle stories I found the compound-specificity question above is not academic. On main the ProfileMenu popover overlaps the trigger button; before the NavBar migration it sat just below it.

Mechanism: pre-#130, ProfileMenuPopover = styled(NavBarPopover) extended a styled component, so styled-components guaranteed the child's margin-top: 0.4rem was injected after the parent's margin-top: -1rem and won. #130 turned NavBarPopover into a plain component with .navbar-popover { margin-top: -1rem } in a real stylesheet. ProfileMenuPopover's styled-components class then tied on specificity and lost on injection order — Vite's stylesheet wins over the styled-components runtime sheet in this build. So the 0.4rem override has been silently dead on main since #130, and the popover moved up 1.4rem.

Verified with Chromium screenshots of the Ladle stories at three refs — 33f379f9c (last commit before #130), main, and this branch:

story / state branch vs pre-#130 main vs pre-#130
profile-menu--with-initials open / hover pixel-identical differs (14px shift)
profile-menu--with-links open / hover pixel-identical differs
profile-menu--with-user-icon-fallback open / hover pixel-identical differs
profile-menu--standalone open / hover pixel-identical differs
all profile-menu closed states pixel-identical pixel-identical
help-menu--default closed / open / hover pixel-identical pixel-identical

So this branch is a byte-for-byte match with the pre-migration styled-components rendering, and main is the outlier. I took that as the intended target, since the parent ticket's success criterion is no visual regression against the styled-components baseline and nothing in #130 suggests the shift was deliberate. But it does mean this PR visibly moves the profile popover down 14px relative to main — if you'd rather keep what's currently deployed, drop the margin-top: 0.4rem line and I'll re-baseline.

HelpMenuItem's :focus-visible override (outline: 0 + background) is the same story by the same mechanism — it is equally dead on main and equally restored here. That one I did not manage to capture in a screenshot; forcing Chromium's :focus-visible on a programmatically focused menu item defeated me, so it is an inference from the popover's demonstrated ordering rather than a measurement.

Two things worth a reviewer's eye

1. Compound selectors for the two real overrides. styled-components resolved .navbar-popover vs ProfileMenuPopover and .navbar-menu-item:focus-visible vs HelpMenuItem:focus-visible by injection order (equal specificity, later class wins). In plain CSS that would depend on stylesheet order, so those two rules are written as .navbar-popover.profile-menu-popover and .navbar-menu-item.help-menu-item:focus-visible — one extra class of specificity makes the outcome order-independent. Both are commented in the CSS. Everything else is a single-class selector.

2. The test diff is additive plus one carried commit. #138 and #139 replaced both components' snapshots with targeted assertions, and this branch leaves both specs exactly as they stand — it only appends describe blocks for the style passthrough and className composition. Those new blocks assert the caller's style reaches the element and the override hook wins, rather than asserting inline defaults, matching the reconciliation #143 made to the CORE-2710 tests. The only non-additive part is the #138 cherry-pick described above, which isn't really this PR's change.

Verification

Not in this PR

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Migrates ProfileMenu and HelpMenu from styled-components to plain CSS while retaining theme-driven values through CSS custom properties.

Changes:

  • Adds plain CSS styling for both menus.
  • Replaces styled wrappers with class-based React components.
  • Stabilizes the ProfileMenu focus snapshot.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
ProfileMenu/ProfileMenu.css Defines ProfileMenu styles.
ProfileMenu/index.tsx Adds class-based wrappers and theme variables.
ProfileMenu/index.spec.tsx Waits for settled focus state.
ProfileMenu/__snapshots__/index.spec.tsx.snap Updates rendered output.
HelpMenu/HelpMenu.css Defines HelpMenu and iframe styles.
HelpMenu/index.tsx Replaces styled wrappers and iframe components.
HelpMenu/__snapshots__/index.spec.tsx.snap Updates rendered output.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/components/ProfileMenu/index.tsx Outdated
Comment thread src/components/HelpMenu/index.tsx Outdated
RoyEJohnson

This comment was marked as resolved.

@OpenStaxClaude
OpenStaxClaude force-pushed the CORE-2006-migrate-profile-and-help-menus branch from 5b92dd5 to 0dcff07 Compare August 31, 2026 14:35
@OpenStaxClaude
OpenStaxClaude changed the base branch from main to CORE-2710-compose-render-props-style August 31, 2026 14:35
@OpenStaxClaude

This comment was marked as resolved.

RoyEJohnson

This comment was marked as resolved.

@OpenStaxClaude

Copy link
Copy Markdown
Contributor Author

@RoyEJohnson understood — unstacked and paused.

Unstacked. The branch is rebased back onto main and no longer carries #137's commit. One snag: GitHub auto-created stack #142 when I set the base to #137's branch, and now the API refuses to move it back — Cannot change the base branch because the pull request is part of a stack — from gh pr edit, from REST, and after a close/reopen. The stack REST resource has no delete or remove-PR endpoint I can reach. So the base ref still reads CORE-2710-compose-render-props-style even though the branch content is main-based. The "Files changed" diff is computed from the merge base with main, so it shows exactly the 8 files and nothing of #137. It should retarget itself when #137 merges and that branch is deleted; if you want it clear sooner, the unstack control in the UI should do what the API won't. Sorry for the detour — I read the stacking as the honest way to express the dependency, not as a house pattern.

What that costs, so the pause is a knowing one. Composing style in ProfileMenuItem/HelpMenuItem genuinely can't land before #137: they hand style to NavBarMenuItem, which spreads it, and composeRenderProps always returns a function — so composing at only one level sends a function into that spread and loses the wrapper's variables and the caller's style. That's worse than today's object-form behaviour, not better. Same for className and #136: composing only here would delete the help-menu-item class outright rather than just an override.

So this round keeps what works against main on its own: ProfileMenuButton and HelpMenuButton compose their style (their style reaches react-aria's Button directly), which is exactly the two places Copilot flagged. The two menu items stay on the spread with a comment naming #137 as the blocker. Six tests for the button wrappers plus object-form coverage for the menu items; 4 of them fail without the fix. Suite is green: 39 suites / 246 tests / 116 snapshots, lint and tsc --noEmit clean, npm run build fine.

Still paused and still a draft. The remaining two gaps are a small commit each once #136 and #137 are on main, and I'll rebase then. The PR description now carries the done / not-done / blocked breakdown so it's readable when you come back to it.

Two open questions whenever you get to them, no rush: the 14px profile-popover position (that one changes what ships), and whether you'd rather I fold the className composition in after #136 or file it as a follow-up ticket.

RoyEJohnson

This comment was marked as resolved.

@OpenStaxClaude

This comment was marked as resolved.

@RoyEJohnson
RoyEJohnson force-pushed the CORE-2710-compose-render-props-style branch from 590bda0 to 9a9bae3 Compare September 1, 2026 19:52
RoyEJohnson

This comment was marked as resolved.

@OpenStaxClaude
OpenStaxClaude force-pushed the CORE-2006-migrate-profile-and-help-menus branch from 5d4e885 to 40d1092 Compare September 1, 2026 19:57
@OpenStaxClaude
OpenStaxClaude changed the base branch from CORE-2710-compose-render-props-style to main September 1, 2026 19:58
@OpenStaxClaude

This comment was marked as resolved.

RoyEJohnson

This comment was marked as resolved.

@OpenStaxClaude
OpenStaxClaude force-pushed the CORE-2006-migrate-profile-and-help-menus branch from 40d1092 to f1942b2 Compare September 1, 2026 20:12
@OpenStaxClaude
OpenStaxClaude changed the base branch from main to CORE-2720-global-css-theme-tokens September 1, 2026 20:12
@OpenStaxClaude

This comment was marked as resolved.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The migration preserves component behavior and public styling hooks with focused regression coverage.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

RoyEJohnson

This comment was marked as resolved.

OpenStaxClaude and others added 4 commits September 2, 2026 20:48
Replaces the styled-components definitions in ProfileMenu/index.tsx and
HelpMenu/index.tsx with plain CSS files, following the hybrid approach used
by the other components in this migration: theme values stay in JavaScript
and are bound to CSS custom properties on the element, with the palette hex
as a fallback in the CSS.

No test changes: the snapshot flake this branch originally worked around is
gone from main, CORE-2715 (#138) and CORE-2716 (#139) having replaced both
component snapshots with targeted assertions. Those specs pass against the
migrated components unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses Copilot's two review comments. ProfileMenuButton and HelpMenuButton
merged the caller's style into their CSS-variable object with a spread.
react-aria-components types style as `CSSProperties | ((renderProps) =>
CSSProperties)`, and spreading a function copies nothing, so a render-callback
style was silently dropped and replaced by the wrapper's static object. Both now
build their variables inside composeRenderProps, the same shape CORE-2710 (#137)
uses for NavBarMenuItem/NavBarPopover/TreeCheckbox. The caller still spreads
last, and the object form is unchanged -- all 116 snapshots pass untouched.

Their style reaches react-aria's Button directly (HelpMenuButton's by way of
NavBarButton, which passes it straight through), so this works against main as
it stands.

ProfileMenuItem and HelpMenuItem have the same defect but are left spreading for
now, with a comment saying why: they hand style to NavBarMenuItem, which spreads
it as well, so composing only here would send a function into that spread and
lose these variables along with the caller's style. Both levels have to compose
together, which needs #137 on main first.

Eight tests: three per button wrapper (render-callback merged, render-callback
overriding the wrapper variables, object form merging caller-last) plus
object-form coverage for each menu item. Confirmed failing before the fix:
stashing only the two component files leaves 4 failed, 33 passed across the two
specs -- the four callback and override cases fail, the object-form cases pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CORE-2708 (#136) is on main now, so NavBarButton, NavBarMenuItem and
NavBarPopover compose className through composeRenderProps. The wrappers added
here can follow: ProfileMenuButton, ProfileMenuPopover, ProfileMenuItem,
HelpMenuButton and HelpMenuItem passed the caller's className straight into
classNames(), which ignores functions, so a render-callback className was
silently dropped -- and unlike the style case TypeScript does not catch it.

Composing only at this level was not possible before #136: the composed function
would have reached NavBarMenuItem's classNames() and been discarded there,
taking the profile-menu-item / help-menu-item class with it.

Two tests per component, matching the shape #136 uses: a render-callback
className reaches the DOM alongside the wrapper's own class, and a string
className still composes. Confirmed failing before the fix: stashing only the
two component files leaves the two render-callback cases red.

The style half of this is still split. The two button wrappers compose it; the
two menu items cannot until CORE-2710 (#137) lands, because NavBarMenuItem
still spreads style. Comments in both files say so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follows the CORE-2720 (#143) sweep, which this branch is now stacked on. The two
new stylesheets repeated eleven palette hexes as var() fallbacks; they now read
the --ox-* tokens instead, e.g.

  color: var(--help-menu-button-color, var(--ox-color-gray));

The override hooks are unchanged -- only their defaults moved from JavaScript to
the CSS side, so the components no longer bind static custom properties inline.
That means style is no longer destructured in ProfileMenuButton,
ProfileMenuItem, HelpMenuButton or HelpMenuItem: it passes through in ...props
and react-aria handles both the object and render-callback forms itself.

Two consequences worth naming:

- The CORE-2710 (#137) dependency is gone rather than deferred. The bug it
  guards against was a wrapper overwriting the caller's style, which these
  wrappers no longer do, so the menu items need nothing from #137. Same
  reasoning as the note #143 leaves on NavBarMenuItem.
- iframeWrapperStyle and putAwayStyle are gone; the iframe wrapper and the
  put-away bar take their colours from HelpMenu.css.

className composition stays -- that one is a real bug fix, not a default.

The specs that asserted the inline defaults now assert what matters instead:
the caller's style reaches the element in both forms, and the override hook
still wins. Defaults are covered centrally by src/theme/tokens.spec.ts, which
also fails on any colour literal that duplicates a theme value -- both new
stylesheets pass it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@OpenStaxClaude

This comment was marked as resolved.

@RoyEJohnson
RoyEJohnson marked this pull request as ready for review September 2, 2026 21:06
@RoyEJohnson
RoyEJohnson requested a review from jivey September 2, 2026 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants