🚨 [CSS Modules] Migrate Button to CSS Modules - #3164
Conversation
Migrate `Button` from Aphrodite to CSS Modules (CSS Modules Phase 4 of epic WB-2120). This is a spike on the worst-case variant matrix (~36 cells across `kind × actionType × size`) to validate the migration patterns before the component waves. The public API is unchanged — same props, DOM structure, and `style` / `styles` / `labelStyle` overrides — so this is an internal styling refactor. `ActivityButton` (which has its own `_generateStyles`) is deferred. The `kind × actionType` colour matrix is expressed with CSS custom-property "slots" set by the `[data-kind]` attribute plus per-`actionType` classes, so each interactive state is declared once instead of once-per-cell. Orthogonal axes (size, pressed, focused) compose through the WB `style` prop via `processStyleList` (no `clsx`). Theming is unchanged — the module references the same `--wb-*` token vars that switch on `[data-wb-theme]`. The shared reset moves to `@layer shared` so it no longer beats the layered component styles. The package now ships `dist/index.css` (auto side-effect import) and exposes it via the new `@khanacademy/wonder-blocks-button/css` subpath. Verified: jest (all 5 button suites), typecheck, eslint, stylelint, and a full rollup build all pass — the build emits `dist/index.css` with `@apply --wb-focus-visible` expanded and the cross-package `@import` inlined under `@layer shared`. Visual parity still needs a Chromatic diff review on this PR. Issue: WB-2328 ## Test plan: Automated checks are green and cover behavior/structure/aria: - `pnpm jest packages/wonder-blocks-button` — all 5 suites pass - `pnpm typecheck`, `eslint`, `stylelint` — clean - `pnpm rollup -c ./build-settings/rollup.config.mjs` — emits button `dist/index.css` (layer-wrapped, `@apply`/`@import` resolved) + injects the `import "./index.css"` side-effect into the ESM/CJS entries Manual (visual — not covered by the above): 1. Review the Chromatic diff on this PR; any diff must be intentional (target: none). 2. In Storybook, spot-check `Button` across `kind` (primary/secondary/tertiary) × `actionType` (progressive/destructive/neutral) × `size` (small/medium/large) and the hover / active / focus-visible / disabled states, in the default, thunderblocks, and syl-dark themes. http://localhost:8228/?path=/docs/packages-button-button--docs ## Review plan: Please review these risky changes 1. 🚨 `packages/wonder-blocks-button/src/components/button.module.css`: New file defining the entire visual matrix (colour slots, per-kind structural rules, hover/press/focus/disabled states) for a shared component used across many apps; correctness of the visual output depends on Chromatic review. 2. 🚨 `packages/wonder-blocks-button/src/components/button-core.tsx`: Rewrites the styling of a widely-consumed shared library component — removes Aphrodite `_generateStyles` and composes CSS Module classes through the `style` prop instead. 3.⚠️ `packages/wonder-blocks-button/src/components/button-unstyled.tsx`: Moves the shared element reset from Aphrodite to a CSS Module in `@layer shared`; the reset intentionally no longer zeroes `border` (the component fully specifies it per kind). 4.⚠️ `packages/wonder-blocks-button/package.json`: Adds `sideEffects` and a new `./css` export subpath, changing how consumers pick up the component stylesheet (standard webpack/Vite/Next pick it up automatically; SSR-without-CSS-loader consumers may need a mock). ### Common patterns: **2 Files (button-core.tsx, button-unstyled.tsx):** Replace Aphrodite `StyleSheet.create` / `_generateStyles` with a colocated `*.module.css` and compose the resulting class-name strings through the Wonder Blocks `style` prop (routed to `className` by `processStyleList`). ```tsx // Before import {StyleSheet} from "aphrodite"; const styles = StyleSheet.create({reset: {position: "relative", /* … */}}); style={[styles.reset, style]} // After import styles from "./button-unstyled.module.css"; style={[styles.reset, style]} ```
🦋 Changeset detectedLatest commit: b06b724 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
🚨 High risk. New file defining the entire visual matrix for Button — a shared component used across many apps. Colour slots (--button-bg/fg/border-*) are set per kind × actionType; structural per-kind rules (primary outline, secondary/tertiary inset box-shadow, tertiary underline); hover/press/focus/disabled states each declared once. Ported faithfully from the earlier spike (which had 0 Chromatic diffs) with all token var names re-verified against the current token build, but visual correctness ultimately depends on the Chromatic diff on this PR.
There was a problem hiding this comment.
🚨 High risk. Rewrites the styling of a widely-consumed shared library component: removes Aphrodite StyleSheet/_generateStyles and composes CSS Module class names through the WB style prop (processStyleList routes strings to className). No _generateStyles export remains, but nothing outside this file imported it (ActivityButton has its own copy). disabled is now selected via [aria-disabled="true"] in CSS rather than a JS-applied class.
There was a problem hiding this comment.
button-unstyled.module.css in @layer shared, so it no longer beats the layered component styles. The reset keeps border: none (as the original Aphrodite reset did) to strip the UA default border. Because the bundle emits .reset after .button at equal specificity, that would otherwise clear the secondary/tertiary rest border — so button.module.css re-asserts the rest border for those kinds at higher specificity via [data-kind] (see the dedicated rule there). Primary is unaffected (zero-width border, uses outline).
There was a problem hiding this comment.
sideEffects: [\"**/*.css\"] and a new ./css → ./dist/index.css export subpath (keeps the existing ./styles.css → token vars). Changes how consumers pick up the component stylesheet: standard webpack/Vite/Next pick up the auto side-effect import with no change; SSR consumers that cannot process CSS imports may need a CSS loader/mock.
|
Size Change: -1.12 kB (-0.85%) Total Size: 130 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (ecac6a1) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR3164"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR3164 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-eagijyimfi.chromatic.com/ Chromatic results:
|
Re-add `border: none` to the `button-unstyled` `.reset` (matching the original Aphrodite reset, which stripped the user-agent default border off the rendered `<button>` / `<a>`). Because the CSS Modules bundle emits `.reset` after `.button` in the same `@layer shared` at equal specificity, an unqualified `border: none` would win and clear the base `.button` border — wiping the secondary / tertiary rest border. To keep that from regressing, re-assert the rest-state border for those kinds at a higher specificity via the `[data-kind]` attribute selector (which also re-declares `border-style`, since `border: none` resets it). Primary is excluded (zero-width border, uses `outline`); hover / press / disabled continue to override as before. Verified with stylelint and a full rollup build (the re-assertion emits at a higher specificity than the reset, so the borders win regardless of source order).
…ctive When a Button is focused *and* hovered or pressed, the focus ring was being painted over by the state styling. Root cause of the regression: the migration qualified the kind-specific hover/active rules with `[data-kind]` (specificity 0,3,0) but left the base `:focus-visible` / `.focused` rule unqualified (0,2,0), so the primary `outline` and secondary/tertiary inset `box-shadow` won over the focus ring. The original Aphrodite styles kept all three at equal specificity with focus declared last, so focus took priority. Fix: qualify the focus rule with `[data-kind]` to restore equal (0,3,0) specificity — it is emitted after hover/active, so the ring wins again for primary and tertiary. The secondary composites (which intentionally combine the inset border shadow with the ring) are extended to also cover the programmatic `.focused` / `.pressed` class states, not just the `:focus-visible` + `:hover` / `:active` pseudo combinations. Verified with stylelint and a full rollup build. Visual parity across the focus + hover / active / pressed combinations for all three kinds still needs the Chromatic diff review on the PR.
…t border Extend the secondary focus composites to `data-kind="tertiary"` so a focused + hovered / pressed tertiary button keeps its inset border `box-shadow` composited with the focus ring, instead of the ring replacing it. This matters in the thunderblocks theme, where tertiary's hover / press border-width is non-zero and the inset border is visible; in the default theme tertiary's border is 0-width, so the composite collapses to the plain ring (no visual change). Covers the `:focus-visible` + `:hover` / `:active` pseudo states and the programmatic `.focused` / `.pressed` classes, matching the secondary handling. Verified with stylelint, a full rollup build, and the button jest suite. Visual parity for tertiary focus + hover / active across themes still needs the Chromatic diff review on the PR.
… simplify focus/border rules - Add `@apply --wb-focus-visible` to the disabled `:focus-visible` rule so a focused disabled button shows the global focus ring. - Back out the `[data-kind]`-qualified focus rule and the secondary/tertiary rest-border re-assertion / tertiary focus composites, returning to the simpler base `.button:focus-visible` / `.button.focused` focus rule.
…ia [data-kind] Qualify the focus rules with [data-kind] so they match the specificity of the kind-specific hover/active rules and win by source order, and document why.
| .button { | ||
| /* Layout (was `sharedStyles.shared`). Height is the medium default; the | ||
| * `.small` / `.large` size classes override it. */ | ||
| block-size: var(--wb-c-button-root-sizing-height-medium); |
There was a problem hiding this comment.
note: These --wb-c-button vars are still coming from the theme/default.ts files as these are transformed to CSS vars during build time.
There was a problem hiding this comment.
Will we be converting the theme styles to css modules too at some point?
There was a problem hiding this comment.
I'm not sure yet as we still have the ergonomics in JS/TS that allow us to merge themes easily. We'll see once we get rid of the OG/default theme.
…en surface Adopt the pattern NodeIconButton established: name every value a variant axis can change as a `--wb-c-button--*` component token, and split the rules into ones that *assign* tokens (one class per axis value) and ones that only *read* them (base + one rule per interactive state). - Variant selection moves off the `[data-kind]` attribute onto `.primary` / `.secondary` / `.tertiary` classes composed by `button-core`, so all three axes (kind, actionType, size) work the same way. `data-kind` stays on the element as a consumer/test hook, it just no longer drives styling. - State rules qualify the kind with `:where(…)`, which adds no specificity. Every state rule now sits at the same specificity, so source order alone decides precedence (hover -> active -> focus -> disabled). That removes the specificity juggling the previous commits needed to keep the focus ring on top of hover/press. - Local slot names (`--button-bg-default`) are renamed to the namespaced `--wb-c-button--bg-default` form, matching `--wb-c-node-icon-button--*` and keeping them distinct from the codegen'd `--wb-c-button-root-*` design tokens. No visual or API change: the declaration blocks are unchanged (verified by diffing all 45 normalized rules against the previous version), and the public props are untouched. A `tokens` prop like NodeIconButton's is deliberately left for a follow-up.
Reworked the styling structure around a component-token surfacePushed
No visual or API change is intended. To check that, I normalized the old and A Verified
Not verified — needs eyes on the Chromatic diffI could not get the Storybook story tests to complete for the button stories |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## deploy/css-modules #3164 +/- ##
==========================================
==========================================
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the reported findings, I checked whether tertiary buttons should also get the focus+hover/press box-shadow compositing that secondary has (the :where(.secondary):focus-visible:hover/:active rules) — the pre-migration Aphrodite code only special-cased kind === "secondary" for that compositing too, so tertiary lacking it here matches prior behavior rather than being a new regression.
Extended reasoning...
Checked the old Aphrodite _generateStyles in the diff: the :focus-visible:hover/:focus-visible:active box-shadow compositing override was only ever applied when kind === "secondary", never for tertiary. The new CSS module preserves that asymmetry (only .button:where(.secondary):focus-visible:hover/:active exist, no tertiary equivalent), so this is behavior-preserving, not a new gap introduced by the migration.
| * `.small` / `.large` size classes override it. */ | ||
| block-size: var(--wb-c-button-root-sizing-height-medium); | ||
| padding-block: 0; | ||
| padding-inline: var(--wb-c-button--padding-inline); | ||
|
|
||
| /* Theming, driven by the colour tokens set by the variant classes. */ | ||
| border-radius: var(--wb-c-button-root-border-radius-default); | ||
| border-style: solid; | ||
| border-width: var(--wb-c-button--border-width-default); | ||
| border-color: var(--wb-c-button--border-default); | ||
| background: var(--wb-c-button--bg-default); | ||
| color: var(--wb-c-button--fg-default); | ||
|
|
||
| /* Animation */ | ||
| transition: border-radius 0.1s ease-in-out; |
There was a problem hiding this comment.
🔴 Secondary and tertiary buttons lose their visible rest-state border. .reset in button-unstyled.module.css sets border: none, and .button's border-style: solid (button.module.css:75-77) is the only remaining border declaration — both are equal-specificity, same-layer rules, so whichever emits later in the bundle wins. An earlier commit on this branch (e01864d) added a [data-kind="secondary"], [data-kind="tertiary"] rule specifically to re-assert this border at higher specificity after empirically verifying (via a full rollup build) that .reset emits after .button; commit 68ea60e removed that protection during a focus-rule cleanup, and the final rework (cd058a3) never restored an equivalent.
Extended reasoning...
This PR moves the Button element reset from unlayered Aphrodite CSS into button-unstyled.module.css's .reset class, inside @layer shared. .reset still contains border: none (kept intentionally, to strip the UA default border — see the comment atop button-unstyled.module.css). The rest-state border for secondary/tertiary buttons is declared once, on the base .button rule in button.module.css:75-77 (border-style: solid; border-width: ...; border-color: ...). Both .reset and .button are single-class selectors living in the same @layer shared, so they have identical specificity (0,1,0) — precedence between them is decided purely by which one is emitted later in the final CSS Modules bundle.
This exact scenario was already discovered and fixed once on this branch. Commit e01864d ("Restore Button element border reset") re-added border: none to .reset to match the original Aphrodite behavior, and in the same commit added a dedicated higher-specificity rule — .button[data-kind="secondary"], .button[data-kind="tertiary"] { border-style: solid; border-width: ...; border-color: ...; } — whose own comment states: ".reset sets border: none in the same @layer shared at equal specificity but later source order, which would otherwise clear the base .button border." The commit message adds that this was "Verified with stylelint and a full rollup build". That protection rule was compound-class-plus-attribute (0,2,0 specificity), so it would win over .reset regardless of emission order.
Commit 68ea60e ("Give disabled Button the global focus ring; simplify focus/border rules") then deleted that entire re-assertion rule as part of a [data-kind] cleanup for the focus rules. Its message only justifies the focus-ring simplification and never addresses why removing the border re-assertion is safe. The final rework in cd058a3 restructured the file around component tokens but never reintroduced an equivalent — grep -n "border-style" on the current file returns exactly one hit, the base .button rule, with no [data-kind], :where(...), or other higher-specificity selector protecting it at rest state. The import order that produced the author's verified build result is also unchanged: button-core.tsx still imports ButtonUnstyled (which pulls in button-unstyled.module.css) and button.module.css in the same relative order as when e01864d measured the emission order.
Proof, step by step:
ButtonUnstyledrenders the DOM node withstyle={[styles.reset, style]}, andButtonCorepassesdefaultStyle(which includesstyles.button) as thatstyle. So both.resetand.buttonland as classes on the same rendered<button>/<a>element.- At rest (no hover/press/focus), the only CSS in scope for
border-styleis.reset { border: none }(→border-style: none) and.button { border-style: solid; ... }. - Both selectors are specificity (0,1,0) in the same
@layer shared; per CSS cascade rules, ties are broken by source order in the final stylesheet, and the author's own rollup-build verification (frome01864d) established that.resetis emitted after.button. - With no higher-specificity rule re-asserting the border (which is exactly what
e01864dadded and68ea60eremoved),.reset'sborder: nonewins, so secondary and tertiary buttons render with no visible border at rest.
Impact: this is a visible regression on a widely-consumed shared component — every secondary and tertiary Button instance across every consuming app would lose its outlined rest-state appearance (primary is unaffected; it uses a zero-width border and relies on outline instead). Hover, press, and focus states are unaffected since they paint the border via box-shadow/outline at higher specificity, so this would likely slip past a cursory visual spot-check that only exercises interactive states. It also isn't caught by CI: the only 0-diff Chromatic build in this PR's timeline ran on 2026-08-03, before 68ea60e (which removed the protection) and long before the final cd058a3 rework — the PR author's own final comment says they could not get story tests to run locally and explicitly asks reviewers to treat the Chromatic diff as the real gate for exactly this kind of specificity behavior, which a static diff can't confirm.
Fix: restore a rest-state border re-assertion for secondary/tertiary at higher specificity than .reset (e.g., re-add the [data-kind="secondary"], [data-kind="tertiary"] rule that 68ea60e removed, or an equivalent :where()-free / attribute-qualified selector), or alternatively drop border: none from .reset now that .button fully specifies the border for every kind (as the very first migration commit cfa9698 originally did, before it was reverted in e01864d to match legacy Aphrodite behavior).
|
|
||
| - The `kind × actionType × size` variant matrix is now expressed in | ||
| `button.module.css` using CSS custom-property "slots" set by the `[data-kind]` | ||
| attribute plus per-`actionType` classes, so each interactive state is declared | ||
| once instead of once-per-cell. Theming (default / thunderblocks / syl-dark) is |
There was a problem hiding this comment.
🟡 The changeset (.changeset/css-modules-phase-4-button.md, lines 9-13) says Button's variant matrix is driven by the [data-kind] attribute plus per-actionType classes, but the shipped implementation (after the cd058a3 rework) selects kind via .primary/.secondary/.tertiary classes instead — the PR's own top-level description was updated to reflect this, but the changeset text was not. Since changeset bodies ship verbatim into the public CHANGELOG, this stale sentence should be updated to describe the class-based approach before merge.
Extended reasoning...
What the bug is. The changeset at .changeset/css-modules-phase-4-button.md (lines 9-13) states that the kind × actionType × size variant matrix is expressed in button.module.css 'using CSS custom-property "slots" set by the [data-kind] attribute plus per-actionType classes.' That sentence describes the button styling approach from the PR's earlier commits (a59410a, 68ea60e), where the kind axis was selected via the [data-kind] attribute selector.
How it was superseded. The final rework commit (cd058a3, per the author's own 2026-08-04 19:23 comment: 'Reworked the styling structure around a component-token surface') replaced the [data-kind]-attribute selection with per-kind classes. In the shipped button.module.css, the kind axis is selected via .primary, .secondary, and .tertiary classes (e.g. .primary { --wb-c-button--border-width-default: ...; }), and state rules qualify kind with :where(.secondary, .tertiary) etc. button-unstyled.tsx still sets data-kind on the DOM element, but purely as a consumer/test hook — the CSS no longer has any [data-kind] selector driving styling. The author explicitly updated the PR's own top-level description to say 'data-kind is still set on the element ... it just no longer drives styling' — but the changeset file itself was never updated to match.
Why nothing else catches this. Changesets are free-form prose maintained by hand; there's no lint or test that cross-checks changeset text against the actual implementation, so a changeset written for an earlier iteration of a PR can silently ship stale once the implementation is reworked in a later commit.
Impact. Changeset bodies are concatenated verbatim into the public CHANGELOG.md at release time. Consumers or future maintainers reading the changelog entry for this migration would be told the styling mechanism is attribute-driven when it is actually class-driven — a factually wrong description of an internal implementation detail.
Proof (step-by-step).
- Read
.changeset/css-modules-phase-4-button.md" lines 10-13: '...expressed inbutton.module.cssusing CSS custom-property "slots" set by the[data-kind]attribute plus per-actionType` classes...' - Read the shipped
button.module.css: the kind axis is set by.primary,.secondary,.tertiaryclass rules (not[data-kind]), e.g..primary { --wb-c-button--border-width-default: var(--wb-c-button-root-border-width-primary-default); ... }. - Read the author's PR comment dated 2026-08-04 announcing commit
cd058a3: 'restructuresbutton.module.cssto follow the patternNodeIconButtonestablished, rather than the[data-kind]-attribute approach the earlier commits used. The PR description is updated to match.' - Confirm the top-level PR description was indeed updated (it now says 'data-kind ... no longer drives styling'), but the changeset file was not touched in that commit — it retains the pre-rework wording.
Suggested fix. Update the changeset bullet to say the matrix is expressed via per-kind, per-actionType, and per-size classes setting --wb-c-button--* component tokens, dropping the [data-kind] attribute reference — mirroring the language already used in the updated PR description.
beaesguerra
left a comment
beaesguerra
left a comment
There was a problem hiding this comment.
Nice work, I'm glad we have visual regression tests to check all the different states! I left some questions I wanted to get your thoughts on (mostly to refresh my memory as you continue this work!)
| - The package now ships its bundled stylesheet at `dist/index.css` (imported | ||
| automatically as a side-effect of the JS entry) and exposes it explicitly via | ||
| the new `@khanacademy/wonder-blocks-button/css` subpath. `sideEffects` is set | ||
| so bundlers keep the side-effect import. Standard webpack / Vite / Next.js | ||
| setups pick this up with no changes; SSR consumers that can't process CSS | ||
| imports may need a CSS loader / mock in their build. |
There was a problem hiding this comment.
Out of curiosity, is perseus already set up to automatically import the new css bundled stylesheet? I think I recall frontend would already handle this?
There was a problem hiding this comment.
Not yet, but I'm planning to create that at some point in the near future, following this frontend approach: https://github.com/Khan/frontend/pull/11912/changes#diff-2d9ab5214985b9a608b4cd47ed561a301cfd4c7d5352c7a94ee046d4e1f1af10
| size === "large" && sharedStyles.large, | ||
| styles.button, | ||
| styles[kind], | ||
| actionType && styles[actionType], |
There was a problem hiding this comment.
Is there type safety when accessing imported styles like this? I think there was, but thought I'd check!
There was a problem hiding this comment.
That's correct! In this case we need to verify that actionType is set as it is an optional prop and CSS modules types the definitions inside that file:
| @@ -0,0 +1,35 @@ | |||
| /** | |||
| * Reset styles shared by the rendered `<button>` / `<a>` / `Link` element of | |||
| * `Button` (CSS Modules migration — WB-2328). | |||
There was a problem hiding this comment.
Did we want to keep the ticket details in the comment still? It isn't necessarily documenting future work / a to-do item so I wonder if we can remove it! (We'll have the css module migration ticket in the commit anyways!)
There was a problem hiding this comment.
good point! I'll remove that.
| * --wb-c-button--border-width-default per-state widths (kind) | ||
| * --wb-c-button--border-width-hover | ||
| * --wb-c-button--border-width-press |
There was a problem hiding this comment.
The naming structure kind of reminds me of bem naming (block, element, modifier). What do you think about following the naming structure separating the element and the modifier? I find it a bit easier to read with different separators!
For example, --wb-c-button__border-width--default,--wb-c-button__border-width--hover, --wb-c-button__border-width--press, etc.
There was a problem hiding this comment.
I left it that way to preserve the CSS variables notation as close as we could. I asked Claude about this and it's aligned with this approach: https://claude.ai/share/aa90711b-998a-44dd-b47c-386f48798ca1
There was a problem hiding this comment.
Thanks for looking into this more! That makes sense, bem is more for DOM nodes, rather than CSS variable names!
This current approach is the "Structural naming order" one right? I remembered we had a structure for naming component tokens, so it makes sense to continue that order! https://khanacademy.atlassian.net/wiki/spaces/WB/pages/3967287319/Component+tokens+structure
| * `kind` only, not `actionType`). | ||
| */ | ||
| .primary { | ||
| --wb-c-button--border-width-default: var( |
There was a problem hiding this comment.
I'm curious about this layer of CSS variable definitions! Is there a reason why we don't define the CSS properties in for the primary style here? (for example, setting border-width: var(--wb-c-button-root-border-width-primary-default) directly here)
There was a problem hiding this comment.
It is mostly because we reuse those "internal" CSS vars in different selectors (e.g. .button, .button[aria-disabled="true"], etc), and these change between kinds and themes.
I think this can be simplified once we get rid of the default/OG theme.
| .button { | ||
| /* Layout (was `sharedStyles.shared`). Height is the medium default; the | ||
| * `.small` / `.large` size classes override it. */ | ||
| block-size: var(--wb-c-button-root-sizing-height-medium); |
There was a problem hiding this comment.
Will we be converting the theme styles to css modules too at some point?
…t branch state The changeset was written at the first migration commit and has drifted over the five follow-ups since. - The variant-matrix bullet still described custom-property "slots" selected by the `[data-kind]` attribute. cd058a3 moved variant selection onto per-axis classes over a `--wb-c-button--*` component-token surface, so that sentence described a mechanism that no longer exists. Rewritten, plus a note that `data-kind` is still emitted as a consumer/test hook but no longer styles anything. - Added the cascade note. This is the change most likely to actually surprise a consumer and it was going out undocumented: Aphrodite emitted Button's rules unlayered and `!important`, the CSS Modules build emits them in `@layer shared`, so unlayered consumer CSS now wins regardless of specificity. Overrides via the `style` / `styles` / `labelStyle` props still route through Aphrodite and are unaffected. Deliberately not mentioned: 68ea60e ("give disabled Button the global focus ring") reads like a behavior change but isn't one relative to `main`. The pre-migration `default` style spread `...focusStyles.focus`, and Aphrodite's `recursiveMerge` merged the disabled `:focus-visible` colours *over* it, so a focused disabled Button already showed the ring. That commit fixed a regression introduced earlier in this branch, so it's invisible to consumers and doesn't belong in a changeset. No code change — changeset copy only. Issue: WB-2328 ## Test plan: Nothing to verify manually; this is documentation-only. `prettier --check` passes on the file, and the changeset frontmatter (`wonder-blocks-button: minor`) is unchanged, so the release plan is unaffected. ## Review plan: No High or Medium risk changes — this touches a single markdown file in `.changeset/` with no effect on shipped code. Worth a skim for accuracy of the cascade claim in the final bullet.
…ayer reset The reset and the component styles both land in `@layer shared` at equal specificity, and the bundler emits this file *after* `button.module.css`, so the reset's `border: none` was winning and wiping the secondary / tertiary rest-state border. Bundle order is not a lever here. `rollup-plugin-styler` sorts the extracted stylesheets by the *last* position each file occupies in a breadth-first walk of the module graph, so a shared dependency like this one — pulled in by both `button-core` and `activity-button` — is always reached late. I tried forcing it with side-effect imports at the package entry and alongside each component's own CSS; both moved the reset *further* down, because every added import path contributes another late occurrence. The plugin exposes no ordering option, and Storybook resolves CSS through Vite rather than this pipeline, so there is no single order to rely on either way. Wrapping the rules in `@layer reset` makes the cascade explicit instead of positional. No build change was needed: `wrap-in-layer` skips only files already wrapped in `@layer shared`, so this file comes out as `@layer shared { @layer reset { … } }` — the sub-layer `shared.reset` — while component styles stay directly in `shared` and outrank it. Issue: WB-2328 ## Test plan: Verified from the emitted `packages/wonder-blocks-button/dist/index.css` that the nesting is what we expect: @layer shared @char 0 <- button.module.css @layer shared @char 19391 @layer reset @char 19406 <- this file Class hashing (`wb-button-unstyled-reset-#`), `*.module.css.d.ts` generation, stylelint, prettier, typecheck and all 5 button jest suites pass. NOT verified, and the reason this needs a human: the fix rests on rules placed directly in a layer outranking that layer's named sub-layers. That is CSS Cascade 5 behaviour, but it could not be confirmed in this environment — Chromium will not launch here and no cascade-layer polyfill was installable, and static analysis cannot settle it because specificity and source order both say the reset should win. It is only observable once an engine renders it. 1. In Storybook, confirm secondary and tertiary Buttons show a visible rest-state border (all themes). http://localhost:6061/?path=/docs/packages-button-button--docs 2. Confirm the same in the Chromatic diff. If those borders are missing, the sub-layer assumption is wrong — fall back to `:where(.reset)`, which drops the reset to specificity (0,0,0) and is provable from the emitted CSS alone. ## Review plan: Please review these risky changes 1.⚠️ `packages/wonder-blocks-button/src/components/button-unstyled.module.css`: Fixes a live regression (missing secondary / tertiary rest border) via cascade-layer semantics that are asserted from the spec rather than observed in a browser. Worth a second opinion on the nested-layer precedence claim, and it must be confirmed visually before this ships.
… changeset The reset bullet still said the reset "lives in `@layer shared` alongside the component styles", which is exactly what e83bfd3 changed — it is now emitted into the nested layer `shared.reset` so the component styles, which sit directly in `shared`, outrank it regardless of bundle order. No code change — changeset copy only. The regression that motivated the layer (secondary / tertiary losing their rest border) was introduced and fixed within this branch, so it stays out of the changeset; `main` never shipped it. Issue: WB-2328 ## Test plan: Documentation-only. `prettier --check` passes and the frontmatter (`wonder-blocks-button: minor`) is unchanged, so the release plan is unaffected. ## Review plan: No High or Medium risk changes — a single markdown file in `.changeset/` with no effect on shipped code.
| @@ -0,0 +1,34 @@ | |||
| /** | |||
There was a problem hiding this comment.
What changed. This reset and the component styles both end up in @layer shared at equal specificity, and the bundler emits this file after button.module.css — so the unqualified border: none was winning and wiping the secondary / tertiary rest-state border. That was a live regression on this branch (introduced by the migration, fixed here; main never shipped it).
Why not just fix the order. Bundle order isn't a lever. rollup-plugin-styler sorts extracted stylesheets by the last position each file occupies in a breadth-first walk of the module graph (dist/index.js:1450, :1469), so a shared dependency like this one — pulled in by both button-core and activity-button — is always reached late. I tried forcing it two ways and instrumented the result:
| Attempt | lastIndexOf ranking |
|---|---|
Side-effect import at the top of src/index.ts |
button 11, unstyled 12, activity 13 |
| Side-effect import beside each component's own CSS | button 14, unstyled 16, activity 17 |
Both pushed the reset further down — each new import path adds another late occurrence. The plugin exposes no ordering option, and Storybook resolves CSS through Vite rather than this pipeline, so there is no single order to rely on either way.
How the fix works. wrap-in-layer skips only files already wrapped in @layer shared, so wrapping these rules in @layer reset yields @layer shared { @layer reset { … } } — the sub-layer shared.reset — with no build change. Verified from the emitted dist/index.css:
@layer shared @char 0 <- button.module.css (directly in `shared`)
@layer shared @char 19391
@layer reset @char 19406 <- this file
The assumption. This only fixes the bug if rules placed directly in a layer outrank that layer's named sub-layers. That is CSS Cascade 5 behaviour, but I could not confirm it here — Chromium won't launch in this sandbox and no cascade-layer polyfill was installable. Static analysis can't settle it either: specificity and source order both still say the reset should win, so the layer is doing all the work. It's only observable once an engine renders it.
So the review ask: confirm secondary and tertiary Buttons show a visible rest-state border, in Storybook and in the Chromatic diff, across all three themes. If those borders are missing, the sub-layer assumption is wrong — the fallback is :where(.reset), which drops the reset to specificity (0,0,0) and is provable from the emitted CSS alone (I built and validated that variant before switching to the layer approach).
2 participants
Migrate
Buttonfrom Aphrodite to CSS Modules (CSS Modules Phase 4 of epicWB-2120). This is a spike on the worst-case variant matrix (~36 cells across
kind × actionType × size) to validate the migration patterns before thecomponent waves. The public API is unchanged — same props, DOM structure, and
style/styles/labelStyleoverrides — so this is an internal stylingrefactor.
ActivityButton(which has its own_generateStyles) is deferred.Styling is organised around a component-token surface, following the
convention
NodeIconButtonestablished with--wb-c-node-icon-button--: everyvalue a variant axis can change is a
--wb-c-button--*custom property, andeach rule does exactly one of two jobs, never both.
.primary,.progressive,.small) sets token values and nothing else, so all threeaxes compose the same way — as class-name strings through the WB
stylepropvia
processStyleList(noclsx).once instead of once-per-cell of the matrix.
Colour is the one place the axes aren't orthogonal — the semantic tokens are
keyed by
kindandactionType(
--wb-semanticColor-action-<kind>-<actionType>-<state>-*) — so those ninecells are spelled out as compound
.primary.progressiverules. Collapsing themwould mean interpolating token names, which only JS can do.
State rules qualify the kind with
:where(…), which contributes nospecificity. Every state rule therefore sits at the same specificity and source
order alone decides precedence, declared hover → active → focus → disabled:
focus has to survive hover/press, and disabled resets everything.
data-kindis still set on the element as a consumer/test hook, but no longer drives
styling.
Theming is unchanged — the module references the same
--wb-*token vars thatswitch on
[data-wb-theme]. The shared reset moves to a CSS Module too, emitted into thenested layer
shared.resetso the component styles — which sit directly inshared— outrank it regardless of the order the bundler emits the twostylesheets in. The package now ships
dist/index.css(auto side-effect import) and exposes it via the new@khanacademy/wonder-blocks-button/csssubpath.Issue: WB-2328
Test plan:
Buttonacrosskind(primary/secondary/tertiary) ×actionType(progressive/destructive/neutral) ×size(small/medium/large) andthe hover / active / focus-visible / disabled states, in the default,
thunderblocks, and syl-dark themes.
that combination is what the
:where(…)specificity flattening protects, forevery
kind(primary'soutline, secondary/tertiary's insetbox-shadow).button visually unchanged.
(all three themes). This one is not covered by CI: the reset now sits in the
nested layer
shared.reset, and that only beats the reset's late position inthe bundle if rules directly in a layer outrank its named sub-layers — CSS
Cascade 5 behaviour that could not be verified outside a browser. See the
comment on
button-unstyled.module.cssfor the fallback if it's wrong./?path=/docs/packages-button-button--docsReview plan:
Please review these risky changes
button.module.cssbutton-core.tsxbutton-unstyled.tsxpackage.jsonbutton-unstyled.module.cssCommon patterns:
2 Files (button-core.tsx, button-unstyled.tsx): Replace Aphrodite
StyleSheet.create/_generateStyleswith a colocated*.module.cssand compose the resulting class-name strings through the Wonder Blocksstyleprop (routed toclassNamebyprocessStyleList).