[ColorSync] Convert matcher font and colors to semantic tokens#3566
[ColorSync] Convert matcher font and colors to semantic tokens#3566
Conversation
🗄️ Schema Change: No Changes ✅ |
|
Size Change: +2 B (0%) 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 (49db39e) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3566If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3566If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3566 |
|
Size Change: +83 B (+0.02%) Total Size: 504 kB 📦 View Changed
ℹ️ View Unchanged
|
…ors to semantic tokens
Myranae
changed the title
![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.
Approving — the production change is a single one-line color token swap with snapshot/Chromatic verification; the inline comments are nit-level (workflow doc paths, story comment staleness, manifest tag) and don't need to gate this.
Extended reasoning...
Overview
This PR is part of the ColorSync migration. The production-code surface area is minimal: packages/perseus/src/widgets/matcher/matcher.tsx adds a semanticColor import and converts the single hardcoded #444 border color to semanticColor.core.border.neutral.strong (resolves to #4a4c53). The snapshot file is updated in lockstep with the changed Aphrodite class hashes. Everything else added in the PR is non-shipped: .claude/ workflow instruction templates, a progress report, plus a new regression-story decorator and an initial-state regression stories file under __docs__/.
Security risks
None. No auth, crypto, network, input-handling, or permission code is touched. The only runtime change is replacing a hex literal with a token reference from @khanacademy/wonder-blocks-tokens.
Level of scrutiny
Low. The production change is a one-line mechanical swap that follows the documented codemod mapping (color.offBlack/#444-equivalent dark-gray border → core.border.neutral.strong), is verified by an updated snapshot, and was confirmed against Figma per the progress report. Chromatic baselines were established and approved before/after the color change. CSS classnames change but the structure is unaffected.
Other factors
The inline bug comments are all nit-level: (1) two workflow-instruction files have stale path references and a hardcoded free-response/ path that will trip up the next run — non-shipped, easy follow-up; (2) the new regression-stories file's per-story comments name #444 literally, which becomes incorrect the moment this PR lands — minor doc staleness; (3) the new *-regression.stories.tsx file is missing !manifest in its tags array, which every other regression-stories file in the repo includes per CLAUDE.md guidance — a real but small convention break. None of these affect runtime behavior or the correctness of the color migration; they're worth a follow-up tweak but don't warrant blocking the merge.
| const meta: Meta<typeof MatcherWidget> = { | ||
| title: "Widgets/Matcher/Visual Regression Tests/Initial State", | ||
| component: MatcherWidget, | ||
| tags: ["!autodocs"], |
There was a problem hiding this comment.
🟡 The new file matcher-initial-state-regression.stories.tsx uses tags: ["!autodocs"] but is missing "!manifest". CLAUDE.md (lines 213-216) explicitly requires visual regression tests (*-regression.stories.tsx) to include "!manifest" in their tags to keep them out of the component manifest, and every other regression stories file in the repo follows this convention. Fix: change tags to ["!autodocs", "!manifest"].
Extended reasoning...
What
The new file packages/perseus/src/widgets/matcher/__docs__/matcher-initial-state-regression.stories.tsx declares its tags as:
tags: ["!autodocs"],This is missing the "!manifest" tag.
Why it is a bug
CLAUDE.md (lines 213-216) explicitly states:
Add
!manifestto non-component stories. Documentation pages, visual regression tests (*-regression.stories.tsx), a11y docs, and dev utilities should include"!manifest"in their tags to keep them out of the component manifest.
The *-regression.stories.tsx filename suffix is exactly the case the rule names. Every other regression story file in the repo follows this convention — across definition, radio, free-response, explanation, numeric-input, image, label-image, and interactive-graphs widgets, the regression story files use one of ["!autodocs", "!manifest"], ["!dev", "!manifest"], or ["!manifest"]. The new matcher file is the only *-regression.stories.tsx in the codebase missing the !manifest tag.
Step-by-step proof
- The file in question is
packages/perseus/src/widgets/matcher/__docs__/matcher-initial-state-regression.stories.tsx— the suffix-regression.stories.tsxmatches the rule. - Line 13 of the file:
tags: ["!autodocs"],— only!autodocsis present;!manifestis absent. - The component manifest (Storybook tooling) reads
tagsfrom each story to decide whether to include the component. Without!manifest, the matcher visual regression stories will be incorrectly registered in the component manifest alongside real components. - Comparison to the
label-imageregression file referenced in the workflow guide shows it includes!manifest— same pattern matcher should follow.
Impact
Manifest hygiene only — the visual snapshot output and Chromatic coverage are unaffected, and runtime behavior in production is not affected. The regression stories will leak into the component manifest, which is a documented project-rule violation and breaks the established convention used by all 17 other regression story files in the repo.
Fix
Change line 13 from:
tags: ["!autodocs"],to:
tags: ["!autodocs", "!manifest"],There was a problem hiding this comment.
@nishasy @mark-fitzgerald Is this a thing? It does seem like other files have this, so adding it.
| generateLabelImageOptions, | ||
| generateLabelImageWidget, | ||
| } from "./utils/generators/label-image-widget-generator"; | ||
| /** @hidden */ |
There was a problem hiding this comment.
Claude mentioned this is here to keep these out of documentation, but I'm not exactly sure when to use this and when not to (and it wasn't sure either). I think this relates to our JSdoc documentation method? Anyone know?
Sounds like all generators except these three are hidden, so maybe these are the outliers:
- generateInteractiveGraphWidget (and all its graph type helpers)
- generateNumericInputWidget
- generateRadioWidget
| left: ["f(x) = 1/x", "f(x) = 1/x²", "f(x) = 1/x³"], | ||
| right: ["f'(x) = -1/x²", "f'(x) = -2/x³", "f'(x) = -3/x⁴"], | ||
| orderMatters: false, | ||
| padding: true, | ||
| } satisfies Partial<PerseusMatcherWidgetOptions>, |
There was a problem hiding this comment.
@mark-fitzgerald I'm thinking I could just replace this with the sharedArgs too. Though the content in shared args isn't relevant, we only care about the labels in this story anyway. What do you think? Last improvement and I'll move onto sorter 😂
There was a problem hiding this comment.
I like how this example shows the use of TeX throughout, not just in the labels. I realize that the styling targets the header, but it would probably be helpful to have TeX in the body to make sure that future styling changes aren't too broad. I recommend keeping this as is. Perhaps change the title to just "WithTeX", and add a comment in the code that says this is primarily for the header.
There was a problem hiding this comment.
There is no TeX in the content. It is just text. The TeX in the cards was makes the story flakey as the spacing is not stable. It changes when it shouldn't change. Claude explained it was because of timing with the snapshots and how the spacing is calculated. "The flakiness is about when Chromatic takes the screenshot relative to the onMeasure height measurement cycle." The below screenshot shows the TeX story spacing (with actual TeX) changing even though nothing in this story was changed (text in another story was changed to cause the snapshots to recapture).
So, I think I'll probably keep the TeX out of the content for now, I can either keep the card content as TeX like text or use the content in the shared args. Or, up for other suggestions as well :) ... Actually, I'll take out the fake TeX. I think it's confusing and unhelpful. More so than if it was just not TeX in the content.
There was a problem hiding this comment.
Added the TeX back with a delay and all is well. Thanks!
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/perseus-editor@31.2.0 ### Minor Changes - [#3571](#3571) [`4e0193210e`](4e01932) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Deprecate `serialize()` function on EditorPage, ArticleEditor, and Editor components - use `onChange` prop instead! - [#3495](#3495) [`0dd432126d`](0dd4321) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Add usePreviewController and usePreviewPresenter hooks for typed iframe preview communication - [#3499](#3499) [`8860d6a838`](8860d6a) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Export preview system hooks and types from @khanacademy/perseus-editor ### Patch Changes - [#3580](#3580) [`e5e93e1dab`](e5e93e1) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Remove some feature flags related to Interactive Graph Phase 2 (absolute value, tangent, logarithm, and exponent) - Updated dependencies \[[`1f4e2b5105`](1f4e2b5), [`cf2e061e24`](cf2e061), [`e5e93e1dab`](e5e93e1), [`138a955dfc`](138a955)]: - @khanacademy/perseus@77.4.1 - @khanacademy/perseus-core@26.0.2 - @khanacademy/keypad-context@3.2.47 - @khanacademy/kmath@2.4.5 - @khanacademy/math-input@26.4.19 - @khanacademy/perseus-linter@5.0.2 - @khanacademy/perseus-score@8.8.1 ## @khanacademy/keypad-context@3.2.47 ### Patch Changes - Updated dependencies \[[`1f4e2b5105`](1f4e2b5), [`e5e93e1dab`](e5e93e1)]: - @khanacademy/perseus-core@26.0.2 ## @khanacademy/kmath@2.4.5 ### Patch Changes - Updated dependencies \[[`1f4e2b5105`](1f4e2b5), [`e5e93e1dab`](e5e93e1)]: - @khanacademy/perseus-core@26.0.2 ## @khanacademy/math-input@26.4.19 ### Patch Changes - Updated dependencies \[[`1f4e2b5105`](1f4e2b5), [`e5e93e1dab`](e5e93e1)]: - @khanacademy/perseus-core@26.0.2 - @khanacademy/keypad-context@3.2.47 ## @khanacademy/perseus@77.4.1 ### Patch Changes - [#3574](#3574) [`1f4e2b5105`](1f4e2b5) Thanks [@handeyeco](https://github.com/handeyeco)! - Make angleOffsetDeg nullable - [#3566](#3566) [`cf2e061e24`](cf2e061) Thanks [@Myranae](https://github.com/Myranae)! - Convert matcher colors to semantic tokens - [#3580](#3580) [`e5e93e1dab`](e5e93e1) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Remove some feature flags related to Interactive Graph Phase 2 (absolute value, tangent, logarithm, and exponent) - [#3582](#3582) [`138a955dfc`](138a955) Thanks [@catandthemachines](https://github.com/catandthemachines)! - Fixing horizontal overflow issues in GIF images. - Updated dependencies \[[`1f4e2b5105`](1f4e2b5), [`e5e93e1dab`](e5e93e1)]: - @khanacademy/perseus-core@26.0.2 - @khanacademy/keypad-context@3.2.47 - @khanacademy/kmath@2.4.5 - @khanacademy/math-input@26.4.19 - @khanacademy/perseus-linter@5.0.2 - @khanacademy/perseus-score@8.8.1 ## @khanacademy/perseus-core@26.0.2 ### Patch Changes - [#3574](#3574) [`1f4e2b5105`](1f4e2b5) Thanks [@handeyeco](https://github.com/handeyeco)! - Make angleOffsetDeg nullable - [#3580](#3580) [`e5e93e1dab`](e5e93e1) Thanks [@ivyolamit](https://github.com/ivyolamit)! - Remove some feature flags related to Interactive Graph Phase 2 (absolute value, tangent, logarithm, and exponent) ## @khanacademy/perseus-linter@5.0.2 ### Patch Changes - Updated dependencies \[[`1f4e2b5105`](1f4e2b5), [`e5e93e1dab`](e5e93e1)]: - @khanacademy/perseus-core@26.0.2 - @khanacademy/kmath@2.4.5 ## @khanacademy/perseus-score@8.8.1 ### Patch Changes - Updated dependencies \[[`1f4e2b5105`](1f4e2b5), [`e5e93e1dab`](e5e93e1)]: - @khanacademy/perseus-core@26.0.2 - @khanacademy/kmath@2.4.5 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2 participants
Summary
Part of the ColorSync migration (LEMS-3487) to align Perseus widget colors with the design system's semantic token layer. Adds initial-state regression stories for Chromatic visual coverage, establishing a baseline for future Matcher and Sortable migrations. No interactions stories here as interactions will be covered by Sortable if possible to test. Also adds test data generators for Matcher since there weren't any before and other widgets use generators.
Issue: LEMS-3501
Test Plan