[WB-2328] 🚨 [CSS Modules] Migrate ActivityButton to CSS Modules - #3168
[WB-2328] 🚨 [CSS Modules] Migrate ActivityButton to CSS Modules#3168jandrade wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 7caf2ea 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 |
|
Size Change: -1.64 kB (-1.26%) Total Size: 129 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (95fd296) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR3168"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR3168 |
| @@ -0,0 +1,380 @@ | |||
| /** | |||
There was a problem hiding this comment.
🚨 High risk — new file defining the whole visual matrix.
This file is the entire visual output for ActivityButton (colour tokens, the chonky box, and the hover / press / disabled / focus states) for a component shipped to every consuming app. Nothing in CI verifies the rendered pixels, so correctness here rests on the Chromatic diff.
What to look at closely:
- The colour matrix (6 compound rules). Each
.progressive.primary-style rule maps tosemanticColor.chonky[actionType][property][kind][state]in the deleted_generateStyles. Worth spot-checking a couple of cells against the old JS — a transposedkind/actionTypewould be invisible in one theme and obvious in another. - Source order is load-bearing. The box state rules all sit at (0,3,0) specificity, so precedence comes from declaration order alone: hover → active → disabled → focus. Reordering any of these blocks silently changes behavior.
.box .icon/.box .labelare compound on purpose.PhosphorIcon(.medium, 2.4rem) andBodyText(.mediumSemi) ship single-class rules into the same@layer shared. Aphrodite's!importantused to settle that contest; without the.boxqualifier the winner would come down to which package's stylesheet loads first.- Intentional behavior change. A disabled button that is both focused and hovered now keeps the full focus ring (
.button[aria-disabled="true"]:focus-visible). Previously the disabled:hoverreset was emitted after:focus-visibleat equal specificity and stripped the outline, leaving only the inner box-shadow — an artifact of Aphrodite's rule ordering rather than a design decision.Buttonalready made the same call in 68ea60e.
Expected Chromatic result: exactly one diff, in the hoverAndFocus cell of the disabled column on the ActivityButton StateSheet. Any other diff needs investigating before this lands.
There was a problem hiding this comment.
Interesting, there wasn't a diff in Chromatic! I checked in Storybook and things seem to be looking okay though! Any thoughts on this?
| @@ -1,26 +1,19 @@ | |||
| import * as React from "react"; | |||
There was a problem hiding this comment.
_generateStyles, the local theme object and the aphrodite / focusStyles / tokens imports are all gone (−235 lines). Styling now comes from activity-button.module.css composed through the style prop — processStyleList routes class-name strings to className, so no clsx.
The behavioural bits worth a careful read:
chonkyStylescollapsed to[styles.box, stylesProp?.box].chonkyPressed/chonkyDisabledno longer exist as JS-toggled classes; the box is styled by descendant selectors from the root (.button:hover .box,.button.pressed .box,.button[aria-disabled="true"] .box). Consumer overrides still win, becausestylesProp?.boxgoes through Aphrodite (unlayered +!important) and beats everything in@layer shared.disabledis now selected in CSS, not JS. The[aria-disabled="true"]attribute comes fromButtonUnstyled, which keeps the element focusable. NoteButtonUnstyledrenders a<button>(not an<a>) wheneverdisabledis set, even with anhref, so the attribute is always present when it needs to be.pressed/focusedstill gate on!disabledin JS, matching the old array exactly — there is no.button[aria-disabled="true"].focusedrule in the module because that combination can't occur.- The plain
chonkyclassName is kept deliberately as a consumer/test hook even though it no longer drives styling — the same treatmentdata-kindgot in theButtonmigration.
Behaviour is covered by the existing activity-button.test.tsx suite (roles, mouse handlers, icon aria-hidden), which passes unchanged. Visual parity is not covered — see the Chromatic note on the CSS file.
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-skvgfsabpe.chromatic.com/ Chromatic results:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## WB-2328 #3168 +/- ##
===============================
===============================
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
08ad472 to
ba5767a
Compare
|
@claude review once |
| @@ -0,0 +1,380 @@ | |||
| /** | |||
There was a problem hiding this comment.
Interesting, there wasn't a diff in Chromatic! I checked in Storybook and things seem to be looking okay though! Any thoughts on this?
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| <> | ||
| {/* NOTE: Using a regular className to be able to use descendant selectors to account for the hover and press states */} | ||
| <View style={chonkyStyles} className="chonky"> | ||
| <View style={chonkyStyles}> |
There was a problem hiding this comment.
🟡 The box's className="chonky" was dropped during the Aphrodite→CSS Modules migration (old: <View style={chonkyStyles} className="chonky">, new: <View style={chonkyStyles}>), even though the changeset, PR description, and reviewer's inline comment all explicitly state it is being kept as a consumer/test hook.
Extended reasoning...
The diff removes className="chonky" from the box View in activity-button.tsx (line 74): the old code was <View style={chonkyStyles} className="chonky">, the new code is <View style={chonkyStyles}> with no className at all. This directly contradicts three places in this same PR where the author states the opposite intent: the shipped .changeset/css-modules-phase-4-activity-button.md says "The box keeps its plain chonky class name in the DOM as a consumer/test hook, it just no longer drives styling"; the PR description repeats this almost verbatim; and reviewer jandrade's inline comment on this file (point 4) says "The plain chonky className is kept deliberately as a consumer/test hook... the same treatment data-kind got in the Button migration." All three assert the class survives the migration — the code says otherwise.
The pattern is also the established convention for this component family: activity-icon-button.tsx:136 and node-icon-button.tsx:196 both still render their box View with className="chonky" specifically so it remains a stable DOM hook. ActivityButton is now the outlier among its siblings, and the only one where the documented behavior and the shipped code disagree.
A counter-argument was raised: the original code comment being deleted here ("Using a regular className to be able to use descendant selectors to account for the hover and press states") suggests chonky was originally just an Aphrodite implementation detail for descendant selectors, not a public contract, and that in this migration those descendant selectors move to the CSS-module .box class instead, making .chonky truly dead weight that was correctly deleted. That would be a reasonable read of the old code in isolation — but it doesn't resolve the contradiction: the author of this exact PR wrote, in the changeset and PR description, that the class is being deliberately retained going forward specifically as a consumer/test hook (not as a styling mechanism). Whether or not that was its original purpose, that is the documented intent for post-migration behavior, and the code doesn't match it.
Impact is bounded and this is why it lands as a nit rather than a blocking issue: styling is unaffected since the CSS Modules rules target .box, not .chonky (Chromatic reports 0 diffs), and no test or story in this repo queries .chonky for ActivityButton. The concrete risk is external: a downstream consumer or e2e test in the Khan frontend that queries the box via .chonky — a pattern the changeset explicitly promises still works — would silently break, and the shipped changeset text becomes factually inaccurate about the DOM output.
Proof:
- Before this PR:
<View style={chonkyStyles} className="chonky">renders<div class="chonky ...">. - After this PR:
<View style={chonkyStyles}>renders<div class="...">with nochonkyclass. - Any selector
.chonky(browser test, snapshot, or consumer CSS override) that matched the box before this PR no longer matches it after. - The changeset merged in this same commit claims the opposite: "The box keeps its plain
chonkyclass name in the DOM."
Fix: re-add className="chonky" to the box View in activity-button.tsx (one line), matching activity-icon-button.tsx and node-icon-button.tsx.
…utton to CSS Modules Migrate `ActivityButton` from Aphrodite to CSS Modules, the piece that was deliberately deferred when `Button` moved over (it had its own `_generateStyles`). The public API is unchanged — same props, DOM structure and `styles.root` / `styles.box` / `styles.startIcon` / `styles.endIcon` / `styles.label` overrides — so this is an internal styling refactor. The new `activity-button.module.css` follows the component-token surface `Button` settled on: every value a variant axis can change is a `--wb-c-activity-button--*` custom property, variant classes only *assign* tokens, and the base + state rules only *read* them. The `actionType × kind` colour matrix is six compound rules (`.progressive.primary`, …) because the semantic tokens are keyed by both axes; disabled colours are keyed by `kind` alone so they live on the kind classes. Structural values (border width, the 6px/8px/0 shadow depths) are written literally rather than codegen'd per theme, matching the note the Aphrodite `theme` object carried — every theme reused them. The "chonky" box no longer gets its state styling from classes toggled in JS. `chonkyPressed` / `chonkyDisabled` are gone; the box is styled by descendant selectors from the root (`.button:hover .box`, `.button.pressed .box`, `.button[aria-disabled="true"] .box`), all at the same specificity so source order alone decides precedence: hover -> active -> disabled -> focus. The plain `chonky` className stays on the element as a consumer/test hook, it just no longer drives styling — same treatment `data-kind` got in `Button`. The icon and label rules are qualified with `.box` on purpose. `PhosphorIcon` (`.medium`) and `BodyText` (`.mediumSemi`) ship single-class rules in the same `@layer shared`, and Aphrodite's `!important` used to settle that contest for us; the compound selector makes the outcome depend on specificity instead of which package's stylesheet happens to load first. One intentional behaviour change: a disabled button that is both focused and hovered now keeps the full focus ring. Previously the disabled `:hover` reset was emitted after the `:focus-visible` rule at equal specificity and stripped the outline, leaving only the inner box-shadow. That was an artifact of Aphrodite's rule ordering, and `Button` already made the same call (68ea60e). Verified: `pnpm jest packages/wonder-blocks-button` (all 5 suites), typecheck, eslint, stylelint, prettier, and a full rollup build all pass — the build emits the ActivityButton rules into the package's `dist/index.css` with `@apply --wb-focus-visible` expanded and everything wrapped in `@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 the 28 `wb-activity-button-*` selectors into `packages/wonder-blocks-button/dist/index.css` Manual (visual — not covered by the above): 1. Review the Chromatic diff on this PR. The only expected diff is the disabled + focus + hover cell of the ActivityButton StateSheet (see above); anything else must be investigated. 2. In Storybook, spot-check `ActivityButton` across `kind` (primary/secondary/tertiary) x `actionType` (progressive/neutral) and the hover / active / focus-visible / disabled states, in the default, thunderblocks and syl-dark themes. Confirm the box still lifts on hover and drops flat on press, and that the `WithCustomStyles` story's overrides (root/box/startIcon/endIcon/label) still take effect. http://localhost:6061/?path=/docs/packages-button-activitybutton--docs ## Review plan: Please review these risky changes 1. 🚨 `packages/wonder-blocks-button/src/components/activity-button.module.css`: New file defining the entire visual matrix (colour tokens, the chonky box, hover/press/disabled/focus states) for a shared component; correctness of the visual output depends on Chromatic review. 2.⚠️ `packages/wonder-blocks-button/src/components/activity-button.tsx`: Removes the Aphrodite `_generateStyles` / `theme` block and composes CSS Module classes through the `style` prop instead. The box's state classes are no longer toggled in JS.
a2c2856 to
7caf2ea
Compare
2 participants
Migrate
ActivityButtonfrom Aphrodite to CSS Modules, the piece that wasdeliberately deferred when
Buttonmoved over (it had its own_generateStyles). The public API is unchanged — same props, DOM structure andstyles.root/styles.box/styles.startIcon/styles.endIcon/styles.labeloverrides — so this is an internal styling refactor.The new
activity-button.module.cssfollows the component-token surfaceButtonsettled on: every value a variant axis can change is a--wb-c-activity-button--*custom property, variant classes only assigntokens, and the base + state rules only read them. The
actionType × kindcolour matrix is six compound rules (
.progressive.primary, …) because thesemantic tokens are keyed by both axes; disabled colours are keyed by
kindalone so they live on the kind classes. Structural values (border width, the
6px/8px/0 shadow depths) are written literally rather than codegen'd per theme,
matching the note the Aphrodite
themeobject carried — every theme reusedthem.
The "chonky" box no longer gets its state styling from classes toggled in JS.
chonkyPressed/chonkyDisabledare gone; the box is styled by descendantselectors from the root (
.button:hover .box,.button.pressed .box,.button[aria-disabled="true"] .box), all at the same specificity so sourceorder alone decides precedence: hover -> active -> disabled -> focus. The
plain
chonkyclassName stays on the element as a consumer/test hook, it justno longer drives styling — same treatment
data-kindgot inButton.The icon and label rules are qualified with
.boxon purpose.PhosphorIcon(
.medium) andBodyText(.mediumSemi) ship single-class rules in the same@layer shared, and Aphrodite's!importantused to settle that contest forus; the compound selector makes the outcome depend on specificity instead of
which package's stylesheet happens to load first.
One intentional behaviour change: a disabled button that is both focused and
hovered now keeps the full focus ring. Previously the disabled
:hoverresetwas emitted after the
:focus-visiblerule at equal specificity and strippedthe outline, leaving only the inner box-shadow. That was an artifact of
Aphrodite's rule ordering, and
Buttonalready made the same call(68ea60e).
Verified:
pnpm jest packages/wonder-blocks-button(all 5 suites), typecheck,eslint, stylelint, prettier, and a full rollup build all pass — the build emits
the ActivityButton rules into the package's
dist/index.csswith@apply --wb-focus-visibleexpanded and everything wrapped in@layer shared. Visualparity 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 passpnpm typecheck,eslint,stylelint— cleanpnpm rollup -c ./build-settings/rollup.config.mjs— emits the 28wb-activity-button-*selectors intopackages/wonder-blocks-button/dist/index.cssManual (visual — not covered by the above):
else must be investigated.
ActivityButtonacrosskind(primary/secondary/tertiary) x
actionType(progressive/neutral) and thehover / active / focus-visible / disabled states, in the default,
thunderblocks and syl-dark themes. Confirm the box still lifts on hover and
drops flat on press, and that the
WithCustomStylesstory's overrides(root/box/startIcon/endIcon/label) still take effect.
http://localhost:6061/?path=/docs/packages-button-activitybutton--docs
Review plan:
Please review these risky changes
activity-button.module.cssactivity-button.tsx