[ColorSync] Convert color values to semantic tokens in label-image#3435
[ColorSync] Convert color values to semantic tokens in label-image#3435
Conversation
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: -7 B (0%) Total Size: 495 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (9097781) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3435If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3435If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3435 |
This reverts commit 2dcfc57.
This reverts commit 9aeb1ef.
Though these colors are not touched during this conversion, we still want regression stories that cover all states of the widget
…colors to semantic tokens for label image
…n vs figma color The Figma design was based on using only one point in all the widgets, so it is the interactive graph point. This project does not have a redesign goal, so the Figma does not reflect how the widget should look.
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 |
…g-font-color-conversion
| marginLeft: 5, | ||
| marginRight: 5, | ||
|
|
||
| background: "rgba(33, 36, 44, 0.32)", | ||
| background: semanticColor.core.border.neutral.default, |
There was a problem hiding this comment.
🔴 The separator dot pseudo-element in instructionsChoice uses semanticColor.core.border.neutral.default for its CSS background property, but border tokens are semantically distinct from background tokens and may resolve to different values in non-default themes (dark mode, high-contrast). The correct token is semanticColor.core.background.neutral.default, consistent with markerIncorrect in marker.tsx and the incorrect pill in answer-pill.tsx.
Extended reasoning...
Wrong semantic token category used for separator dot background
What the bug is: In label-image.tsx line 866, the instructionsChoice style block uses semanticColor.core.border.neutral.default as the value of the CSS background property for the ::after pseudo-element that renders separator dots between instruction choices. Border tokens and background tokens are distinct categories in a design token system and are not interchangeable — they exist for different semantic purposes and can resolve to different hues, opacities, or color values across themes.
The specific code path: The instructionsChoice style defines a ::after pseudo-element that creates a 2×2px filled circle (via borderRadius: 2) used as a visual separator dot between each non-last choice item. This is purely a filled background element. The PR replaces the original rgba(33, 36, 44, 0.32) with semanticColor.core.border.neutral.default — a token that belongs to the border category, not the background category.
Why existing code does not prevent this: There is no type-level enforcement preventing a border token from being passed to a background CSS property; both are plain string values at runtime. The bug is a semantic/intent mismatch, not a type error.
Impact: In the default light theme, border and background neutral tokens may happen to be visually similar, so the bug could go unnoticed. However, in dark mode or high-contrast themes, these tokens are likely to diverge. Since the stated purpose of this PR is improved theme support via semantic token conversion, using the wrong token category directly undermines that goal. The separator dots could render with incorrect color or opacity in non-default themes.
Proof by example: Consider the pattern established within this same PR:
marker.tsx:286—markerIncorrectstyle:background: semanticColor.core.background.neutral.default✅answer-pill.tsx:91—incorrectstyle:backgroundColor: semanticColor.core.background.neutral.default✅marker.tsx:213—markerIconborder:border: \2px solid ${semanticColor.core.border.neutral.default}`` ✅ (correct: border token for an actual CSS border)answer-pill.tsx:12— box shadow:boxShadow: \0 8px 8px ${semanticColor.core.border.neutral.default}`` ✅ (correct: border token for a shadow outline)label-image.tsx:866— separator dot:background: semanticColor.core.border.neutral.default❌ (wrong: border token for a fill/background)
All three neutral-colored filled elements use background.neutral.default; the only other uses of border.neutral.default are for actual border and boxShadow properties. Line 866 breaks this consistent pattern.
Fix: Replace semanticColor.core.border.neutral.default with semanticColor.core.background.neutral.default at label-image.tsx:866 to match the pattern used for all other neutral-colored background elements in this widget.
| "@khanacademy/perseus": patch | ||
| --- | ||
|
|
||
| Convert font and colors to semantic tokens for label image |
There was a problem hiding this comment.
🟡 The changeset description says "Convert font and colors to semantic tokens for label image", but this PR only converts color properties — no font-related tokens are changed anywhere. This will produce a misleading CHANGELOG entry; the description should be updated to "Convert hardcoded color values to semantic tokens for label image".
Extended reasoning...
The changeset file at .changeset/nervous-moons-roll.md (line 5) reads: Convert font and colors to semantic tokens for label image. However, examining every modified file in this PR reveals that only background color and border color properties were converted to semantic tokens — no font-related properties (font-size, font-weight, font-family, text color, etc.) were touched.
The specific color conversions made in this PR are:
answer-pill.tsx:backgroundColor: "#00880b"→semanticColor.core.background.success.stronglabel-image.tsx:background: "rgba(33, 36, 44, 0.32)"→semanticColor.core.border.neutral.defaultmarker.tsx:backgroundColor: "#ECF3FE"→semanticColor.core.background.instructive.subtleandbackground: "#00880b"→semanticColor.core.background.success.strong
All four converted values are background or border color properties. None of them are font or typography tokens. The PR title itself confirms this scope: "[ColorSync] Convert color values to semantic tokens in label-image" — no mention of fonts.
The git branch name tb/testing-font-color-conversion and the git log entries (e.g., "Change background to foreground color", "Revert to straight token conversion vs figma color") suggest that font/foreground color conversion was originally planned or experimented with during development, but was ultimately not included in this PR. The changeset description was likely written when font conversion was still planned and never updated when that work was removed.
The impact is purely a documentation inaccuracy: the generated CHANGELOG entry for @khanacademy/perseus will claim that font tokens were converted when they were not. This could confuse consumers reviewing the changelog trying to understand the scope of this patch.
To fix this, the changeset description should be updated to accurately reflect what was done: Convert hardcoded color values to semantic tokens for label image.
| // including TeX fraction choices and the rgba(33, 36, 44, 0.32) separator dots | ||
| // that appear between each choice. | ||
| export const WithChoicesInInstructions: Story = { | ||
| args: { |
There was a problem hiding this comment.
🟡 The WithChoicesInInstructions story comment on line 42 still reads "the rgba(33, 36, 44, 0.32) separator dots" — the exact hardcoded value this PR replaces with semanticColor.core.border.neutral.default. Update the comment to reference the semantic token instead.
Extended reasoning...
This PR replaces rgba(33, 36, 44, 0.32) with semanticColor.core.border.neutral.default in label-image.tsx (line 863). The new story file label-image-initial-state-regression.stories.tsx was written as part of this same PR to serve as a visual regression baseline, but the comment on the WithChoicesInInstructions story was never updated to reflect the token-based implementation.
The stale reference appears at lines 41-42 of the new file:
// including TeX fraction choices and the rgba(33, 36, 44, 0.32) separator dots
// that appear between each choice.
This is immediately contradicted by label-image.tsx where the instructionsChoice::after pseudo-element now uses background: semanticColor.core.border.neutral.default instead of the raw RGBA value.
The comment was almost certainly written before the color conversion was made and was not revisited. Since the file is brand new (added in this PR), there is no pre-existing history to blame — the inaccuracy was introduced the moment this PR was created.
Step-by-step proof: A developer reads WithChoicesInInstructions to understand what it tests. The comment says the separator dots use rgba(33, 36, 44, 0.32). They search the codebase for that string and find nothing, because this PR deleted it. They are now misled about what color value is used. The actual color is resolved at runtime from semanticColor.core.border.neutral.default, a value that may differ across themes.
The fix is a one-line comment update: replace rgba(33, 36, 44, 0.32) with semanticColor.core.border.neutral.default in the story comment. No functional code is affected.
2 participants
Summary:
As the first step of our reignited Color Sync project, we're converting label image's color values to semantic tokens.
#00880b,#ECF3FE) and onergbavalue (rgba(33, 36, 44, 0.32)) inanswer-pill.tsx,marker.tsx, andlabel-image.tsxwith semantic tokens from@khanacademy/wonder-blocks-tokenslabel-image-initial-state-regression.stories.tsxandlabel-image-interactions-regression.stories.tsx) to establish a baseline before conversion and capture diffs afterIssue: LEMS-3994
Test plan:
pnpm lint,pnpm tsc, andpnpm testall passinstructive.default)success.strong)neutral.default)