[ColorSync] Convert label-image widget to CSS Modules#3444
[ColorSync] Convert label-image widget to CSS Modules#3444
Conversation
🗄️ Schema Change: No Changes ✅ |
|
Size Change: -237 B (-0.05%) Total Size: 502 kB 📦 View Changed
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (2dabe95) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3444If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3444If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3444 |
Myranae
force-pushed
the
tb/stacked-css-module-conversion
branch
from
db2383e to
8ddabc2
Compare
|
Something is off about these regression snapshots. That will take a bit more investigation. |
…mage widget Aphrodite styles to CSS modules across answer-pill, hide-answers-toggle, marker, and label-image.
…erence; remove before land
Myranae
force-pushed
the
tb/stacked-css-module-conversion
branch
from
2d5494e to
ebb2a1a
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review once |
There was a problem hiding this comment.
Deferring to human review — mark-fitzgerald's earlier concern about the regression snapshots ("Something is off about these regression snapshots. That will take a bit more investigation") doesn't appear to have been explicitly resolved in the timeline, and that's the primary correctness signal for a visual-only refactor like this.
Extended reasoning...
Overview
This PR converts the label-image widget's Aphrodite styles to CSS modules across 5 files (label-image, marker, answer-pill, hide-answers-toggle, plus the marker story). It's part of an ongoing CSS-modules migration (LEMS-4028 / LEMS-3815). The change adds 3 new *.module.css files, 2 *_legacy-styles.js style-object files (for Wonder Blocks components that take style props rather than className), a changeset, and replaces StyleSheet.create/css(...) call sites with CSS-module styles.X references composed via classNames(...). No behavioral logic was modified.
Security risks
None. This is a styling-only refactor — no auth, data flow, network, or input-handling code is touched. The CSS variables introduced (e.g. --wb-semanticColor-core-background-success-strong) are tokens already in use elsewhere in the project.
Level of scrutiny
Moderate. The mechanical parts (token names, class application via classNames) are straightforward, but two things warrant a human eye: (1) the marker animation/keyframes were translated from Aphrodite's nested animationName object to a top-level @keyframes marker-pulsate rule — small differences here can cause subtle visual regressions; (2) marker-icon modifier classes rely on CSS source order for the cascade, which the file comment acknowledges. Both are exactly the kind of thing Chromatic snapshot review is meant to catch.
Other factors
- mark-fitzgerald's earlier inline comments were addressed (legacy import switched, story
<Wrapper>inlined,label-image_legacy-styles.jsdeleted), but his last standalone comment ("Something is off about these regression snapshots") is the only one without a clear resolution in the timeline before the May 1 review re-request. Recent commits ("Fixed cascade issue", "Fix incorrect css var conversion", "Fix incorrect mapping from conversion") suggest the author has been iterating on this, but the original reviewer hasn't come back to confirm. - The bug-hunter found only nits (dead
*.module.cssfiles for two Wonder-Blocks-only components, and one leftoverimport type {CSSProperties} from "aphrodite"in label-image.tsx). None block merge but all are easy cleanups before landing. - The PR is part of a series following a documented pattern, so the structural choices are consistent with prior CSS-module migrations in the repo.
|
@claude review once |
There was a problem hiding this comment.
Thanks for the cleanup — the lingering aphrodite type import is gone and the orphaned CSS module files are now annotated with their LEMS-3815 follow-up rationale. LGTM.
Extended reasoning...
Overview
This PR converts the label-image widget's Aphrodite styles to CSS modules across five files (label-image, marker, answer-pill, hide-answers-toggle, plus a story file). It also adds two parallel *_legacy-styles.js files for components whose only consumers are Wonder Blocks elements that take a style prop (not className). A changeset is included.
Security risks
None. This is a pure styling refactor — no auth, crypto, data flow, or external surface is touched. Type-only and class-name-only changes.
Level of scrutiny
Low. The change is mechanical and follows an established CSS-module migration pattern already exercised across this package. The widget's behavior is unchanged; the only logic-adjacent edits are swapping css(styles.x) for styles.x and replacing one Wonder Blocks <View style={[...]}> with a <div className={classNames(...)}> in marker.tsx, which matches the surrounding pattern.
Other factors
- My earlier review left two pieces of feedback; both have been addressed in the latest revision: the lingering
import type {CSSProperties} from "aphrodite"is gone (replaced withReact.CSSProperties), and the two orphan.module.cssfiles now carry explicitTODO (LEMS-3815)comments documenting the partial-conversion intent until WB supports CSS module classNames. - Mark Fitzgerald's prior inline review comments (the inline-styles refactor on the story file, the rem conversion on label-image.module.css, and the deletion of
label-image_legacy-styles.js) have all been resolved in the diff. - Chromatic snapshots near-zero per the PR description; size delta is -237 B.
- The author has explicitly requested re-review (
@claude review once).
|
Still has aphrodite code that can't be removed, so holding off on this until the WB conversion to CSS Modules finishes. |
3 participants
Summary:
Converts the label-image widget's Aphrodite styles to CSS modules as part of the ongoing CSS module migration. This is a pure refactor with no intended visual or behavioral changes.
Issue: LEMS-4028
Test plan: