v3.0.0: migrate to Topiary tokens, remove Emotion, ship types and release prep - #112
Merged
Merged
Conversation
Plan for v3.0.0: rebuild react-mega-menu on @jasonrundell/topiary components and tokens, removing Emotion. Includes Topiary audit checklist, --rmm-* token contract draft, phased migration, demo upgrades (React 19 / Next 15), and a11y release gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds slideDirection prop ('left' | 'right', default 'left') as the one
additive API change: direction modifier classes selecting keyframe
pairs in the shipped stylesheet, tested at the Menu seam, exercised
via a demo toggle.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Audit of @jasonrundell/topiary v4.0.1 source and compiled output against the migration plan checklists: atom verdicts for Link, Heading, Button, Row, Box; token vocabulary and theme robustness findings; 7 evidence-backed gaps filed as topiary#151-#157. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Version 3.0.0-alpha.0, react/react-dom peers ^19.0.0 (Emotion peers kept until the migration ticket). Build now emits dist/style.css (./style.css export) with the initial --rmm-* token block, hardcoded fallbacks only. New styleContract smoke test resolves the path from package.json exports and asserts every documented token; pretest builds so npm test is self-sufficient. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
All seven gaps shipped via topiary#158; #94 now targets ^4.1.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds @jasonrundell/topiary ^4.1.0 as a peer + dev dependency and moves react/react-dom dev deps to ^19.0.0 (Topiary requires React 19), ahead of the styling migration in #94. Adds Topiary to the Vite external/ globals map alongside the still-present Emotion externals. Fixes the one genuine React-19 test break: React 19 calls a mocked function component with (props, undefined) instead of (props, {}). Full suite verified green on React 19 (45/45) before any component changes were made. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rewrites every token in src/styles/style.css from a bare hardcoded value to var(--topiary-<real-name>, <same hardcoded value>), using the token names confirmed against @jasonrundell/topiary@4.1.0's compiled dist/style.css (installed in Step 1) rather than the provisional names in the Phase 2 plan draft. Two tokens compose multiple Topiary primitives per the gap report's guidance (no single shorthand exists in Topiary for either): --rmm-focus-ring (borderWidth-thick + focus color) and --rmm-panel-border (borderWidth-hairline + borderStyle + border color). Extends the token surface for values the migration will need that weren't in the Phase 2 draft: panel background/radius, muted text, z-index layers (menu/panel/list), structural chrome sizing (menu/ topbar/logo/hamburger dimensions, routed through the space scale via calc() since Topiary has no direct analog), a generic spacing scale, typography (font family, title/nav-item/description sizes, font weights), and motion (easing, hamburger transition durations). Each new token is added to src/styles/rmmTokens.js with a one-line purpose comment, keeping the contract test (src/styleContract.test.js) as the enforcement point. Fallback values are unchanged from before this commit, so the menu still renders identically with Topiary's stylesheet absent. No component yet consumes these tokens — Emotion still styles every component; this is pure token-contract groundwork for Step 3. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First component-migration cluster (Phase 3a of docs/refactor/topiary-
migration.md). Menu.jsx's StyledMenu becomes a plain div.rmm__menu;
TopBar and Logo become plain elements carrying new rmm__topbar/
rmm__logo classes; TopBarTitle adopts Topiary's Heading atom (level=1,
matching today's <h1>) per the gap-report's ADOPT verdict, now that
topiary#151's className-merge fix has shipped in 4.1.0. All four
translate their old Emotion template literals into rules in
src/styles/style.css against these classes, consuming only the
--rmm-* tokens wired in the previous commit.
Menu.jsx's className handling changes from a self-clobbering pattern
(`className={props.className}` immediately overwritten by a trailing
`{...props}` spread that included the same className) to an explicit
merge: `id`/`className` are destructured out of props so the rmm__menu
class always survives regardless of what the consumer passes, fixing
a latent bug while preserving the public id/className prop contract
(including the pre-existing behavior of defaulting Nav's and MainList's
id to the Menu's own custom id when one is supplied).
Hamburger, Nav, and the list/item components are untouched here and
still styled via Emotion — full suite verified green after this
cluster (45/45) before moving to the next.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…HTML Second component-migration cluster (Phase 3b): MainList, MegaList, NavList, MainNavItem, NavItem, and NavItemDescription drop their Emotion styled.* wrappers for plain ul/li/p elements. Per the gap report's Row/Box verdicts (neither is a fit for this skeleton), these stay plain HTML rather than adopting a Topiary layout atom. MainList and NavList's default (non-"sub") shape were already byte-for-byte identical Emotion rules sharing the caller-supplied "rmm__nav-list" class, so both are now driven by one shared .rmm__nav-list selector in style.css instead of two copies. NavList's "sub" (nested dropdown) shape and MegaList each get their own open/ closed state modifier classes (rmm__nav-list--open/--closed, rmm__mega-list--open/--closed) computed internally from the existing activeState prop, replacing the Emotion activeState-keyed style functions. Both consume a new pair of shared slide keyframes (rmm-slide-open/rmm-slide-closed) added to style.css; Nav itself picks these up too once its own animation moves off Emotion in the next step. Reduced-motion and the large-breakpoint "no animation" overrides are preserved per the original per-component rules (MegaList still hides entirely when closed at the large breakpoint; the nested sub dropdown, like Nav, stays always visible there). Removes a latent bug this migration surfaced: MainNavItem received an `isChildren` prop (helpers/menu.jsx's renderMegaMenuItem) that was never in its propTypes and was never consumed — Emotion's styled.li silently filtered it from the DOM, but a plain <li> spreading ...props does not, so it started firing a "React does not recognize the `isChildren` prop" console warning. Removed at the call site since the prop was dead code. Full suite verified green (45/45), no console warnings, after this cluster. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Third component-migration cluster (Phase 3c), completing Phase 3's component table. NavItemLink and MainNavItemLink now render Topiary's Link atom (component children map to Link's label prop; forwards href/onClick/onKeyDown/aria-*/role/id/className exactly as before); MainNavItemLink's conditional mega chevron span is passed as Link's children, rendered after label per Link's own [label, children] order. Hamburger now renders Topiary's Button atom (non-primary/secondary variant), with the slice container and optional text label passed as children — Button's own label prop is left empty so the exact original DOM (ids rmm__hamburger-slices/--label/--label--container) is preserved rather than reshaped into Button's (label, children) slots. Both adoptions were blocked until topiary#151 (className merge) and, for Button, topiary#157 (children slot) shipped — both landed in the 4.1.0 release this migration targets. style.css gets the corresponding rules. Link's own compiled class already styles color/hover/underline from real --topiary-* tokens with no fallback; this file instead drives color, hover background, and the :focus-visible ring from the --rmm-link-* / --rmm-focus-ring tokens (two-class .rmm__menu .rmm__<x> selectors so they win over Link's own class regardless of import order), so the menu still renders sane values with Topiary's stylesheet absent. Button's own class carries padding/border/box-shadow/gap/radius chrome that fights an icon-only toggle, reset the same way; its own :focus-visible outline is left alone since that's a real accessibility feature, not decorative chrome. Hamburger's state prop drives rmm__hamburger--open/--closed, replacing the Emotion state-keyed style function — translated to match the runtime behavior of the original code exactly (only state === 'open' triggered the X-morph; state === '' was a no-op due to an operator- precedence accident in the old `state === '' || (state === 'open' && ...)` expression, not a second "open-like" state as it visually reads). One deliberate, called-out deviation from pixel-for-pixel parity: the pre-migration Emotion styles never set a background-color on the hamburger's slice spans, so the icon bars were literally invisible. Slices now paint with --rmm-menu-text so the only mobile menu toggle is actually visible; this is flagged in the migration's final report as an intentional fix, not a silent behavior change. Full suite verified green (45/45), no console warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deletes src/helpers/animationStyles.js (Emotion css``/keyframes) and moves Nav's slide-open/close animation to style.css keyframes driven by state classes: rmm__nav--open/--closed (replacing the activeState- keyed Emotion style function) and the new rmm__nav--slide-left/ --slide-right direction modifiers implementing issue #64. Nav becomes a plain <nav> carrying these classes; slideDirection defaults to 'left', reproducing today's translate3d(-100%) behavior exactly, and Menu gets the corresponding additive `slideDirection` prop (PropTypes oneOf(['left','right'])), threaded straight through to Nav. The right variant mirrors the transform pair (rmm-slide-open-right/ rmm-slide-closed-right keyframes) and repositions Nav's resting state (right: -100% instead of left: -100%) so it parks off the opposite edge. Both directions honor prefers-reduced-motion (animation disabled, final transform applied directly) and both are inert at the large breakpoint (nav always visible, no animation), matching Nav's pre-migration behavior exactly. MegaList and the nested sub-dropdown (migrated in the previous commit) already reuse the left-slide keyframes — only the top-level Nav supports a direction choice, since issue #64 scopes the feature to "which side the mobile menu slides in from," not the nested panels. Tests written before the implementation (TDD, confirmed red first): src/components/Nav.test.jsx is rewritten from Emotion-output `toHaveStyle` assertions (which only worked because Emotion injected inline styles jsdom could read — a plain CSS class has no computed style in jsdom) to DOM/class assertions per the migration's rendered- Menu-seam testing approach: open/closed class presence, slide-left default, slide-right opt-in. src/Menu.test.js gains three seam tests covering the same behavior through the public Menu API (default slide-left, slideDirection="right", and open state class on hamburger toggle). Full suite: 7 suites / 47 tests passing (down one suite from the deleted animationStyles.test.js, up 3 net tests from the new coverage above). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Step 5. src/config/breakpoints.js is now the only place the "large"
breakpoint value is written.
JS side (src/helpers/responsive.js): replaces the hardcoded
`viewportLarge = 1024` with isLargeViewport(), which prefers a real
window.matchMedia('(min-width: 64rem)') check (tracks browser zoom/
user stylesheets the way a raw innerWidth comparison can't) and falls
back to comparing window.innerWidth against a pixel-equivalent
breakpoint in environments without matchMedia (jsdom in tests doesn't
implement it — confirmed empirically before writing the fallback
path). `viewportLarge` is kept as an export, now derived from
breakpoints.js (rem-to-px at the standard 16px root) rather than a
second hardcoded number. Menu.jsx's and MenuContext.jsx's mobile-
detection effects (previously two separate copies of the same
`window.innerWidth >= 1024` check) both switch to isLargeViewport();
Menu.jsx also attaches the media query's own 'change' listener where
available, alongside the existing resize listener, so breakpoint
crossings are caught precisely rather than only on generic resize.
CSS side (vite.config.js): the copy-rmm-stylesheet plugin now
substitutes every __RMM_BP_LARGE__ placeholder in src/styles/style.css
with breakpoints.large['min-width'] at build time (custom properties
can't drive @media conditions, so this has to be a literal
substitution, not a --rmm-* token like everything else in the
stylesheet), and throws if any placeholder survives the substitution.
src/styleContract.test.js gains two tests against the built
dist/style.css: no unreplaced __RMM_BP_LARGE__ placeholder, and every
`@media (min-width: ...)` rule matches breakpoints.large['min-width']
exactly.
Full suite: 49/49 passing (up 2 from the new styleContract tests).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Step 6, the last piece of #94's scope. No component has imported Emotion since the three migration-cluster commits; this removes it from the remaining non-component surfaces: - package.json: drops @emotion/react and @emotion/styled from peerDependencies (they were never a devDependency — only ever a peer — so `npm install` after this change removed all 19 now-unused packages from node_modules). - vite.config.js: drops both from rollupOptions.external and the UMD globals map. - src/helpers/responsive.js: deletes respondTo(), the Emotion `${respondTo('large')} { ... }` template-literal interpolation helper — dead now that every component's styling lives in style.css. isLargeViewport/largeBreakpointQuery/viewportLarge (added in the previous commit for the JS/CSS breakpoint single source of truth) are unaffected and remain exported. Verified: `grep -ri emotion src package.json vite.config.js` returns only a historical comment in style.css (documenting that one CSS rule was carried over verbatim from "the pre-migration Emotion" styles) — no imports, no dependency entries. `grep -i emotion dist/index.es.js dist/index.cjs.js` after a clean build returns nothing. Full suite: 49/49 passing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Discovered while building the v3 visual harness (#94 Step 8): the migration ticket's own decisions record "MenuProvider/context exports" as part of the public API that must stay stable across this refactor — but src/index.jsx only ever imported MenuProvider to use internally (the exported `Menu` already wraps its own MenuProvider) and never re-exported it. A consumer following the README's own documented pattern of composing a custom tree around `<MenuProvider>` had no way to import it; the harness app hit this immediately as an unresolved named import. Adds `export { MenuProvider, useMenu }` alongside the existing Menu exports — purely additive, no change to existing behavior. `Menu` from the public entry still works exactly as before (self-contained, wraps its own provider); MenuProvider/useMenu are now also available for consumers who want to build their own tree around the context directly. Full suite: 50/50 passing (up 1 from the new export-presence test in src/index.test.js). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two real bugs found while building and visually verifying the v3 harness (#94 Step 8) against real Topiary tokens (not just the hardcoded fallbacks the Jest suite exercises): 1. --rmm-hamburger-top was var(--topiary-space-2xl, 5.25rem) — an unrelated space-scale token that happens to exceed the topbar's own height with the fallback value, but isn't guaranteed to in every theme. Confirmed visually: under hangar's real tokens (topiary-space-2xl: 3rem) the hamburger landed at top:3rem, inside the 4rem-tall TopBar, overlapping the logo/title. Now derived from --rmm-topbar-height + --rmm-space-md instead of an independent token, which guarantees clearance under any theme by construction. 2. The large-breakpoint rule that turns off Nav's slide animation (`.rmm__nav--open, .rmm__nav--closed { animation: none; ... }`) is single-class specificity (0,1,0), while the slide-direction+state selectors that set the animation (`.rmm__nav--slide-left.rmm__nav--closed { animation-name: ...}`) are 2-class (0,2,0). The higher-specificity rule always wins regardless of source order, so the large-breakpoint override never actually took effect: at desktop widths Nav kept the mobile slide animation's final transform, leaving it translated off-screen (or, depending on state, stuck mid-slide) instead of docked inline. Confirmed by inspecting computed style in the harness at 1300px width: `transform: matrix(1,0,0,1,-1300,0)` with Nav still visually absent. Fixed by matching the override's specificity to 2-class selectors (also applied to the equivalent prefers-reduced-motion block, which had the identical mismatch). Both were invisible to the Jest suite, which only ever exercises the stylesheet's hardcoded fallback values (jsdom can't compute CSS) — a real limitation of the migration's test strategy, called out explicitly in the final report. Full suite: 50/50 passing (no test changes needed; these are pure style.css fixes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…c entry" This reverts commit 40dcd98.
Code review fix (item 2): the `[...].filter(Boolean).join(' ')` idiom
for composing a stable rmm__* class with an optional consumer className
was repeated verbatim in 9 files (Menu, Nav, Hamburger, NavItemLink,
NavList, MegaList, TopBar, Logo, TopBarTitle), and the
`state === 'open' ? 'X--open' : 'X--closed'` ternary recurred in Nav,
MegaList, NavList, and Hamburger.
Adds src/helpers/classNames.js exporting classNames(...parts) (filters
falsy fragments, joins with a space — a direct extraction of the
repeated idiom, not a reimplementation) and stateClass(base, state)
(returns `${base}--open` when state === 'open', `${base}--closed`
otherwise). stateClass's binary collapse matches every call site's
existing behavior exactly, including Hamburger's state prop where ''
(not just 'closed') must map to "--closed" — preserved rather than
templating the raw state value in, which would have produced a
malformed "rmm__hamburger--" class for state=''.
All 9 files now call these helpers instead of inlining the idiom.
Behavior is unchanged — verified both by a dedicated unit test
(src/helpers/classNames.test.js, including a test that classNames'
output matches the pre-extraction idiom byte-for-byte) and by the full
suite staying green with the same 8 suites passing before and after.
Full suite: 63/63 passing (up 9 from the new classNames.test.js).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code review fix (item 3). --rmm-panel-bg, --rmm-panel-border, --rmm-panel-shadow, --rmm-panel-radius, --rmm-menu-border, --rmm-font-weight-regular, and --rmm-space-lg were declared in both src/styles/style.css and src/styles/rmmTokens.js but consumed by no rule — confirmed by grepping for var(--<token>) uses before removing anything (each had exactly one occurrence in the stylesheet: its own declaration). Removes all seven from both files, keeping the contract test's source of truth in sync; token count goes from 38 to 31. The four panel-chrome tokens (bg/border/shadow/radius) were dead for a real reason worth documenting rather than just deleting: the pre- migration Emotion styles never painted a background, border, or shadow on the mega/sub-menu panel either, so leaving the panel unstyled is intentional parity with the original appearance, not an oversight. Added a note to style.css's header comment recording this as an open design question for a later ticket, so it doesn't read as an accidental gap next time someone audits the token list. Adds a regression guard to src/styleContract.test.js: every declared --rmm-* custom property in the shipped stylesheet must be referenced by at least one var(--rmm-*) use elsewhere in the same file. Verified the assertion is meaningful (not a tautology) by temporarily injecting an unused token into a built dist/style.css and confirming the test fails, then reverting. Full suite: 59/59 passing (down from 63 — this commit doesn't include the separate keyframe regression tests from the #64 investigation, committed next). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code review flagged a suspected inversion in slideDirection="right": measured in the harness at 390px with real Topiary tokens, "open" appeared to end off-screen and "closed" on-screen — the opposite of "left"'s behavior. Investigated by reproducing the measurement in a real browser (a throwaway static page loading the built dist/style.css, outside the harness's node_modules) rather than trusting either the original report or my own first read of the source. The initial reproduction attempt showed the same apparent inversion — but forcing the CSS animations to completion via the Web Animations API (`element.getAnimations().forEach(a => a.finish())`) and re-measuring gave the opposite, *correct* result: closed lands off-screen right (1960px in a 980px-wide test), open lands on-screen (x=0), matching the left variant's mirror exactly. document.visibilityState was "hidden" and every animation's currentTime was frozen at 0 regardless of real elapsed wait time — the browser throttles the animation timeline for a tab that isn't genuinely foregrounded, so an unforced read samples the *start* keyframe of whichever animation is assigned, which looks exactly like an inversion between open and closed without being one. A control measurement of all four state × direction combinations (left/right × open/closed), all forced to completion, confirmed every combination already resolves correctly against style.css as committed — no keyframe or positioning change was needed. Per the review's own request, adds a regression test that doesn't depend on a live animation timeline at all: src/styleContract.test.js parses the raw stylesheet source, brace-balances each @Keyframes block, and asserts the translate3d percentage in its terminal `to` rule — rmm-slide-open ends at +100%, rmm-slide-closed at -100%, rmm-slide-open-right at -100%, rmm-slide-closed-right at +100%. Verified the assertions are meaningful (not tautological) by temporarily flipping one keyframe's `to` value in a built dist/style.css and confirming the corresponding test fails, then reverting. Full suite: 63/63 passing (up 4 from these keyframe tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Wires jest-axe's toHaveNoViolations matcher into the shared Jest setup and adds src/accessibility.test.js, which renders the default-exported Menu (real MenuProvider, not mocked) and runs axe in four states: closed/mobile, open/mobile, closed/desktop, open/desktop. All four are green against the current Topiary-styled markup — axe reports zero violations (one aria-valid-attr-value check comes back "incomplete" for aria-controls targets, a known jsdom limitation verifying cross-root getElementById lookups; the referenced ids are present in the rendered DOM, and jest-axe's toHaveNoViolations only inspects violations, not incomplete checks). Regressions in ARIA wiring will now fail this suite same as any other Jest test, satisfying #95's "scans run as part of the normal test suite" criterion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The #94 closing comment flagged the Hamburger as having no aria-expanded, aria-controls, or aria-label — pre-existing since v2, not caught as an axe violation under jsdom (the button already has an accessible name from its visible "Menu" text), but a real gap against the WAI-ARIA disclosure pattern (SC 4.1.2) that #94 explicitly called out for this ticket to close. - Hamburger now sets aria-expanded from its `state` prop and accepts an `ariaControls` prop, forwarded to the underlying Topiary Button (which spreads rest props onto the native <button>, verified against the 4.1.0 build). - aria-label falls back to "Menu" only when no visible `label` is rendered, so consumers who omit `label` still get an accessible name instead of losing the visible-text one to a duplicate. - Menu.jsx computes a single `navId` shared by Nav and the Hamburger's aria-controls, so the toggle is always wired to the region it actually expands/collapses. - Menu.test.js gains three assertions: aria-controls points at the Nav id, and aria-expanded reflects both the closed and open megaMenuState. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Discovered while executing the #95 manual keyboard walkthrough: pressing Escape correctly closed every open panel (resetMenus() already worked), but left focus wherever it happened to be inside the now-closed, invisible panel — dropping to <body> if that element was no longer reachable, rather than landing back on the item that opened it. Confirmed live in the harness (http://localhost:5183) before and after this change. Menu.jsx's window keydown handler now walks up from document.activeElement to the enclosing .rmm__main-nav-item and refocuses its .rmm__main-nav-item-link before calling resetMenus() — a11yEscape's resetMenus() collapses every open level at once, so the top-level MainNavItem link is the one well-defined, always-focusable "triggering item" to return to, regardless of how deep the focused element was nested. This is handler-only: no new props, context state, ARIA attributes, or DOM structure. Focus is left alone when Escape is pressed with focus already outside the menu. Two new Menu.test.js cases cover both branches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds docs/accessibility/keyboard-walkthrough.md — a repeatable checklist covering tab order (forward/reverse), Enter/Space opening a mega panel, Escape closing and returning focus, outside-click close, visible focus in all four Topiary themes, Hamburger reachability/ operability at mobile width, and no keyboard trap. Executed once against this branch's build (commit 3eec3e2) in a live harness (package rebuilt, packed, and reinstalled into a throwaway Vite app; npm run dev on port 5183, driven via the browser tooling and stopped afterward). Results are recorded in the doc: - 11 of 13 rows PASS, including outside-click-close and focus-visible in all four themes (hangar/broadsheet/arcade/cascade), each with the actual computed outline recorded. - 2 rows (Space opening a panel; Hamburger Enter/Space activation) are recorded NOT RUN for the live keypress specifically, with the reason: this session's browser-automation key-press action doesn't populate legacy keyCode/which in this environment, which the app's a11y helpers and native <button> default-action handling both key off. Corroborated instead via the existing a11y.test.js unit coverage and a direct .click() confirmation; flagged for a human to confirm with a physical keyboard. - The Escape row surfaced a real focus-management defect, fixed in commit 3eec3e2 and re-verified live after rebuilding. - One Escalation is recorded and deliberately NOT fixed here: at mobile width, closed off-canvas nav content stays in the tab order (only transformed off-screen, not display:none/inert), so a keyboard user tabs through many invisible links before reaching real content. Fixing it needs isMobile plumbed into Nav — a real interface change, not a handler-level tweak — so it's left for a follow-up ticket. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
npx prettier --check flagged jest.setup.js, src/Menu.test.js, and docs/accessibility/keyboard-walkthrough.md (the Markdown table) against this repo's .prettierrc. No behavioral change — formatting only, per the ticket's final-gate check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code review of the #95 work found the desktop scans in accessibility.test.js were redundant: rendering <Menu /> at 375px and 1280px in jsdom produces byte-identical container.innerHTML for the plain "closed" and "open" states. isLargeViewport() does correctly flip with the width (verified via its innerWidth fallback, since jsdom has no window.matchMedia) — but isMobile is consumed only by MenuContext's toggleSubMenu, and neither of those two states ever calls it, so the "desktop" scans were re-validating the mobile DOM. Adds a third state, "sub-open": open the top-level nav, open Store's mega panel, then click the "back" link inside its Outdoors sub-item's NavList — a second call to toggleSubMenu with a different menuId than the first. toggleSubMenu is the only context action gated on isMobile: at desktop it replaces activeMenus outright with [menuId] whenever the target isn't already active (evicting whatever was open — a de-facto accordion); at mobile it goes through updateActiveMenus, which only adds/removes that one id, and here that update is a no-op because the shared subMenuState toggle already flipped on the previous click. Confirmed genuinely different end states: mobile: #rmm-mega-list-id-store --open (unchanged) #rmm-nav-list-id-store-outdoors --closed (unchanged) desktop: #rmm-mega-list-id-store --closed (evicted) #rmm-nav-list-id-store-outdoors --open (opened) A new test pins that divergence directly (mobile/desktop innerHTML for "sub-open" must differ), and a comment on the test.each table honestly notes that "closed"/"open" stay width-blind re-validations until #100 lands (a width-dependent `inert` attribute), rather than claiming those four scans are more meaningful than they are. The four original tests plus the two new "sub-open" cases are now one test.each table over [widthLabel, width, state] instead of four near-identical test() bodies, with readable generated names like "mobile 375px — sub-open state has no axe violations". Also switches the hamburger lookup from container.querySelector ('#rmm__hamburger') to screen.getByRole('button', { name: 'Menu' }), so this test exercises the accessible-name contract wired in 52bf159 rather than just the DOM id (Menu.test.js's id-selector idiom is left as-is per review — this only applies to the axe seam test). Fixed one bug while building this: the three sequential clicks in the "sub-open" setup were originally wrapped in one shared act(), which under React's batching made the 2nd and 3rd toggleSubMenu calls both read the pre-click closure and clobber each other — collapsing mobile and desktop to the same end state and masking the real divergence. Each fireEvent.click is now its own statement; fireEvent already flushes/commits per call on its own. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code review flagged that Menu.jsx's window keydown handler re-checked e.keyCode === 27 even though helpers/a11y.js's escape() already checks the same thing — two predicates for "is this Escape" that could drift out of sync. helpers/a11y.js now exports isEscape(e), used internally by escape() and imported by Menu.jsx for its own focus-return branch (added in 3eec3e2), so there's exactly one definition of "Escape" shared by both call sites. No behavioral change. New a11y.test.js cases cover isEscape directly (keyCode 27 -> true, anything else -> false). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The demo's install workflow (README's Vercel deploy included) depends on a committed tarball rather than a registry publish. Add a `pack:demo` script (build + npm pack) so that tarball is reproducible instead of a one-off manual `npm pack`, and commit the v3.0.0-alpha.0 tarball it produces. The 2.2.2 tarball stays in place for next-demo until #97. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Point the demo at the v3 tarball (jasonrundell-react-mega-menu-3.0.0-alpha.0.tgz) and its React 19 / @jasonrundell/topiary peers; remove @emotion/react and @emotion/styled, which the v3 package no longer uses. The package's package.json declares "types": "dist/index.d.ts" but its build has no dts step, so nothing ships there — add a minimal local module declaration (demo/src/rmm.d.ts) covering the Menu props this demo actually uses (config, id, className, slideDirection) rather than generating types in the root package, which is follow-up work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Import both stylesheets at the app root (Topiary, then the menu's own --rmm-* stylesheet) and wrap the menu and page content together in a data-theme wrapper, so a Topiary theme switch (hangar / broadsheet / arcade / cascade) re-skins both from identical markup - the page's own CSS (App.css) reads the same --topiary-* tokens directly. Keep the existing ?theme= query-param support, now validated against the four Topiary theme names. Add a slideDirection (left/right) toggle passed straight to <Menu>, and swap the dead via.placeholder.com logo URL for the local /images/logos/logo.svg already in public/. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The five pre-v3 demo themes (light/dark/monokai/retro/synthwave) were
written against v2's Emotion-generated classes ([class*='css-'] selector
gymnastics reaching into rmm__ internals directly: topbar height/padding,
hamburger geometry, icon swaps via content: url(...), border widths).
None of that maps onto v3's DOM or its documented --rmm-* token contract
(src/styles/rmmTokens.js in the package root), so rewrite each theme as a
small block of --rmm-* overrides (color + type only: menu bg/text, link
color, hover bg, muted text, font family, focus ring) scoped to
.rmm__menu.rmm__theme--<name>, applied by passing that class to <Menu
className>.
Dropped in the conversion: each theme's icon set
(demo/public/images/icons/<name>/angle-{left,right,up}.svg, now removed
as dead assets) and its chrome-geometry tweaks (topbar height/padding,
hamburger spacing, border widths) - none of those have a --rmm-* token to
route through; that's Topiary's job now, not a per-theme concern.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the Vite/React/TS boilerplate README with a real one: what the demo shows (Topiary tokens through the --rmm-* contract, no Emotion), how to run it (npm run pack:demo at the repo root, then npm install here), the four-theme switcher and ?theme= param, and the slideDirection toggle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
README.md claimed `npm run build` does "typecheck + production build via Vite", but the script was just `vite build` - Vite doesn't typecheck. Make the claim true: build is now `tsc -b && vite build` (the Vite React + TS template's own default), which was already the case for tsconfig.app.json/tsconfig.node.json project references, just never wired into the script. Also add `*.tsbuildinfo` to demo/.gitignore - tsc -b's own output files were showing up as untracked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
demo/src/rmm.d.ts's local declaration for @jasonrundell/react-mega-menu typed each item's `type` field as `string`, wider than what the component actually understands. Export MenuItemType = 'main' | 'link' | 'mega' | 'sub', mirroring src/config/menuItemTypes.js (MENU_ITEM_TYPES) in the package root, and use it in MenuConfigItem. App.tsx's menuConfig already typechecks against it unchanged - it's annotated `: MenuConfigShape`, so each item literal's `type` string is contextually checked against the union directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
App.tsx repeated `theme.charAt(0).toUpperCase() + theme.slice(1)` at both the Topiary theme buttons and the token-override theme buttons. Extract a small capitalize() helper and use it at both call sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bumps next to 15.5.25, eslint-config-next to match, eslint to ^9, and react/react-dom to ^19 so next-demo can install the v3 package (which peers on React 19). Points @jasonrundell/react-mega-menu at the committed v3.0.0-alpha.0 tarball and adds @jasonrundell/topiary ^4.1.0 per v3's peer dependency. Removes the now-unreferenced v2 tarball at the repo root — nothing installs from it after this change (demo/ already moved to the v3 tarball in #96). Part of #97. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the create-next-app boilerplate with the actual demo: layout.js (server) imports @jasonrundell/topiary/style.css then @jasonrundell/react-mega-menu/style.css so the menu is fully styled on first server paint, page.js (server) reads and validates the `?theme=` search param (awaited, per Next 15's async searchParams) against the four Topiary theme names, and SiteShell.js (the 'use client' boundary) owns theme/slideDirection state and renders <Menu /> plus the page content under one data-theme wrapper — mirroring the Vite demo's theme-switcher convention so the two stay consistent. Also: - menuConfig.js and topiaryThemes.js hoisted as plain modules shared between the server (validation) and client (buttons) sides. - Local SVG logo (copied from demo/public) replaces the dead via.placeholder.com URL; the remote nextjs.org <Image> icons are dropped along with the boilerplate copy they illustrated. - next-demo/.eslintrc.json gets `"root": true`. Without it, ESLint's legacy config resolution cascaded up to the repo root's .eslintrc.cjs (extends `standard`, which pulls in eslint-plugin-n), and that plugin's n/handle-callback-err rule calls the ESLint 9-removed context.getScope(), crashing `next build`'s lint step. - next.config.mjs pins outputFileTracingRoot to silence Next 15's "inferred workspace root" warning, triggered by unrelated lockfiles in ancestor directories. Part of #97. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documents what the demo actually shows now (App Router SSR, the server/client split, stylesheet import order in the root layout, the server-validated ?theme= param), how to run it, and the tarball reinstall workflow shared with demo/'s README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
After the SSR rewrite, nothing in next-demo used a Tailwind utility except the `antialiased` class on <body> in layout.js. Removes the whole toolchain rather than leaving it as unused weight: - globals.css: drop the three @tailwind directives and the unused .text-balance utility; keep the real global styles. Replace `antialiased` with the equivalent plain CSS (-webkit-font-smoothing: antialiased; -moz-osx-font-smoothing: grayscale;) on body, and drop the class from layout.js. - Delete tailwind.config.js and postcss.config.mjs — Next handles CSS fine without a PostCSS config once Tailwind is gone. - Remove tailwindcss and postcss from devDependencies and reinstall (347 -> 304 packages) so package-lock.json reflects it. README.md doesn't mention Tailwind, so no doc change needed. Verified: `npm run build` and `npm run lint` clean (no PostCSS/Tailwind errors); `next start -p 3097` still serves data-theme="hangar" on `/` with --rmm-menu-bg present in the delivered stylesheet, confirming the package stylesheets render correctly with no PostCSS pipeline at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Brings the #100 fix (PR #104) onto the v3 branch: Nav now takes `isMobile` and renders `inert` while closed below the `large` breakpoint, re-applied on the plain-<nav> v3 component as a boolean attribute (React 19). Also brings the upload-artifact@v4 workflow fix and the Playwright keyboard-walkthrough harness. Conflict resolution: - src/components/Nav.jsx, src/Menu.jsx: v3 code plus the inert wiring. - src/accessibility.test.js: v3 axe matrix kept; #104's focus-walk suite appended and the "until #100 lands" note updated. - docs/accessibility/keyboard-walkthrough.md: v3 checklist kept; a dated re-run of row 13 appended and Escalation 1 marked resolved. - package.json / lockfiles / next-demo manifest / migration plan doc: v3 side kept (React 19, Next 15, jest-axe 11, slideDirection plan). main's dependabot bumps for the v2 demos were not carried over. - scripts/a11y-walkthrough/main.jsx: imports Topiary's and the menu's stylesheets so the harness renders the v3 build faithfully. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
A custom `id` was stamped on the shell, the Nav and the main list at once: invalid HTML, and since #95 the Hamburger's aria-controls resolved to the shell instead of the region it toggles. `deriveMenuIds(id)` now gives the shell the id verbatim and derives `<id>__nav` / `<id>__main` for the inner regions; with no `id` the defaults (`rmm__menu`, `rmm__nav`, `rmm__main`, `rmm__hamburger`) are byte-for-byte unchanged. aria-controls always follows the Nav's rendered id. Tests at the rendered-Menu seam: default ids, derived ids, no shared ids, aria-controls with a custom id, and an axe scan with the duplicate-id rules enabled. axe files duplicate-id-aria failures under `incomplete`, which toHaveNoViolations ignores, so the gate checks that bucket too and a tripwire test proves the pre-#101 markup is caught. README and PropTypes describe the derivation. Closes #101 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
Restores the untouched prose that prettier had reflowed, and notes that a custom id must not reuse one of the fixed rmm__* ids. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
package.json has declared "types": "dist/index.d.ts" since v2 but the Vite build never produced it, so TypeScript consumers got "could not find a declaration file" and the Vite demo carried a local shim. - src/index.d.ts: hand-written public type surface — MenuProps (config, id, className, slideDirection: 'left' | 'right', plus div attributes), the named MenuConfigShape / MenuConfigTopbar / MenuConfigItem types and the MenuItemType union; the default export is typed as Menu. - vite.config.js copies it to dist/index.d.ts on every build, next to style.css. The "." export lists the "types" condition first. - The built-in sample config moves to src/config/defaultMenuConfig.js with a JSDoc @type against the declaration (helpers/menu.jsx re-exports it, so imports are unchanged). - src/typesContract.test.js checks the "types" field and export condition agree and resolve to a built file, then runs tsc over test/types/menu.fixture.tsx — a consumer of the shipped declaration with positive cases and @ts-expect-error negatives — and over the JSDoc-typed default config. Loosening the declaration or drifting the config both fail the suite. - demo: src/rmm.d.ts removed; the demo now typechecks (tsc -b) against the tarball's own types. Tarball repacked and both demo lockfiles refreshed via the documented clean reinstall. - README gains a TypeScript section; @types/react added as a root devDependency for the fixture; jest coverage skips .d.ts files. publint and arethetypeswrong resolve the types condition for bundlers, node10 and node16 ESM. Their remaining warning is the pre-existing CJS bundle shipping as .js inside a "type": "module" package. Closes #102 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
… helper Review follow-ups for #102: topbar.logo and menu.items are now .isRequired in Menu.propTypes, matching src/index.d.ts and the unconditional dereferences in render. vite.config.js's two copy plugins share one distPath() helper instead of repeating the mkdir guard. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
…-release Release prep for #98 (everything short of publishing and tagging): - README rewritten for the v3 architecture: install with the React 19 and Topiary peer ranges, the two stylesheet imports and data-theme, a config walkthrough and props table (slideDirection, id, className), the full --rmm-* token table (name, purpose, default), element ids, TypeScript, explicit v2-to-v3 migration notes (what must change, what stays, what is new), and v2.x marked as the React 18 / Emotion maintenance line. - src/readmeContract.test.js holds the README to the stylesheet: every token in rmmTokens.js must be documented with the default style.css declares, and the peer ranges, imports and required sections must be present. - Version bumped to 3.0.0-beta.0; tarball repacked and both demos re-pointed at it with clean reinstalls (both build). - docs/release/v3.0.0.md: the maintainer's publish / verify / tag checklist, including the open CJS-extension decision and the v2 maintenance branch. - Migration plan Phase 5 README gate ticked. Refs #98 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
Review follow-ups for #98: the section regex in readmeContract.test.js now asserts the Usage heading and the slideDirection props row separately (the alternation had unanchored its second branch), and the migration notes tell v2 consumers who targeted a custom id on the nav or main list to move to <id>__nav / <id>__main. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
The package is "type": "module", so the CommonJS bundle emitted as index.cjs.js was read as ESM by Node and flagged by publint and arethetypeswrong. Vite now emits index.cjs, the build copies the declaration to both index.d.ts and index.d.cts, and the "." export uses nested import / require conditions with "types" first in each. typesContract.test.js asserts the new shape: both conditions present, types first, .cjs + .d.cts for require, every entry emitted, and the two declarations identical. publint reports no warnings and arethetypeswrong is green for node10, node16 CJS, node16 ESM and bundler; require() of the package from a CJS script resolves Menu. Tarball repacked and both demo lockfiles refreshed. The release checklist now uses the repo's unprefixed tag convention (2.2.2 exists already) and drops the CJS decision. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ation-plan # Conflicts: # next-demo/package-lock.json
…tier agree eslint-config-standard's space-before-function-paren and prettier disagree on named function declarations; the two helpers now follow the arrow-function style used everywhere else in src. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
This was referenced Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The v3 migration branch (spec #90): the menu's styling moves from Emotion to one shipped plain-CSS stylesheet driven by a documented
--rmm-*token layer that defaults to Topiary tokens with hardcoded fallbacks. React 19 only; Emotion peer deps removed;@jasonrundell/topiaryadded as a peer. TheMenuprops, config shape and everyrmm__*id/class stay stable, so a consumer's only migration work is dependencies, stylesheets and React 19.47 commits. The branch is up to date with
main(merged 69a9e08) and carries the #100inertfix re-applied on the v3Nav.What's in it
--rmm-*token contract, plain-navskeleton with state classes,slideDirectionprop (responsive direction effect #64), stylesheet keyframes withprefers-reduced-motion, a single breakpoint source shared by CSS and JS, both demos on React 19 (Vite; Next 15 App Router as the SSR proof) with a four-theme switcher.npm test; a recorded keyboard walkthrough with a committed Playwright harness; the closed off-canvas nav isinertat mobile width.idprop is applied to three elements, producing duplicate ids #101: a customidno longer lands on three elements; inner regions derive<id>__nav/<id>__main, andaria-controlsfollows the nav's real id. Axe duplicate-id rules are enabled in the gate with a regression tripwire.src/index.d.tsshipped asdist/index.d.tsanddist/index.d.cts; typedMenuPropsand exported config types; atscfixture in the test run catches drift in both directions; the demo's local shim is gone.dist/index.cjswith nestedimport/requireexport conditions,typesfirst in each. publint reports no warnings; arethetypeswrong is green for node10, node16 CJS, node16 ESM and bundler.data-theme, full token table, props, migration notes, v2.x as the React 18 line), contract-tested against the stylesheet; version3.0.0-beta.0with both demos verified against the repacked tarball; maintainer checklist indocs/release/v3.0.0.md.Verification
npm test: 11 suites, 105 tests (axe gate, style / types / README contracts, TS fixture).demo:tsc -bandvite buildclean.next-demo:next buildclean.Not in this PR
Publishing to npm and tagging
3.0.0are manual steps for the maintainer; seedocs/release/v3.0.0.md. #98, #90 and #64 stay open until the release ships.Closes #101
Closes #102
Refs #98, #90, #64
🤖 Generated with Claude Code
https://claude.ai/code/session_017NynwxzG7ZKWBaHW4QAmcp
Generated by Claude Code