-
Notifications
You must be signed in to change notification settings - Fork 364
[ColorSync] Convert color values to semantic tokens in label-image #3435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 16 commits
7e64bc2
3e8cac7
2dcfc57
122de80
9aeb1ef
39416bc
a6d1e15
47b8a24
f2b5e81
449198a
b740a05
275fefb
4bc562d
9095183
cd3ebbd
6c3cdd8
9097781
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@khanacademy/perseus": patch | ||
| --- | ||
|
|
||
| Convert font and colors to semantic tokens for label image | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import {generateTestPerseusItem} from "@khanacademy/perseus-core"; | ||
|
|
||
| import {themeModes} from "../../../../../../.storybook/modes"; | ||
| import {ServerItemRendererWithDebugUI} from "../../../testing/server-item-renderer-with-debug-ui"; | ||
| import { | ||
| incorrectAnswerQuestion, | ||
| textQuestion, | ||
| numberline, | ||
| } from "../__tests__/label-image.testdata"; | ||
|
|
||
| import type {Meta, StoryObj} from "@storybook/react-vite"; | ||
|
|
||
| const meta: Meta = { | ||
| title: "Widgets/Label Image/Visual Regression Tests/Initial State", | ||
| component: ServerItemRendererWithDebugUI, | ||
| tags: ["!dev"], | ||
| parameters: { | ||
| docs: { | ||
| description: { | ||
| component: | ||
| "Regression tests for the Label Image widget that do NOT " + | ||
| "need any interactions to test.", | ||
| }, | ||
| }, | ||
| chromatic: {disableSnapshot: false, modes: themeModes}, | ||
| }, | ||
| }; | ||
| export default meta; | ||
|
|
||
| type Story = StoryObj<typeof ServerItemRendererWithDebugUI>; | ||
|
|
||
| // Verifies the default unanswered state: all markers visible and pulsating, | ||
| // no answers selected, text choices. | ||
| export const DefaultUnanswered: Story = { | ||
| args: { | ||
| item: generateTestPerseusItem({question: textQuestion}), | ||
| }, | ||
| }; | ||
|
|
||
| // Verifies choices shown in the instructions section (hideChoicesFromInstructions: false), | ||
| // including TeX fraction choices and the rgba(33, 36, 44, 0.32) separator dots | ||
| // that appear between each choice. | ||
| export const WithChoicesInInstructions: Story = { | ||
| args: { | ||
|
Comment on lines
+41
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The Extended reasoning...This PR replaces The stale reference appears at lines 41-42 of the new file: This is immediately contradicted by label-image.tsx where the 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 The fix is a one-line comment update: replace |
||
| item: generateTestPerseusItem({question: numberline}), | ||
| }, | ||
| }; | ||
|
|
||
| // Verifies the incorrect marker state: marker dot renders with neutral | ||
| // background (background.neutral.default) when showCorrectness is "incorrect". | ||
| // No answer pill is shown because no answer is selected in this static state. | ||
| export const IncorrectMarker: Story = { | ||
| args: { | ||
| item: generateTestPerseusItem({question: incorrectAnswerQuestion}), | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| import {generateTestPerseusItem} from "@khanacademy/perseus-core"; | ||
| import {within} from "storybook/test"; | ||
|
|
||
| import {themeModes} from "../../../../../../.storybook/modes"; | ||
| import {ServerItemRendererWithDebugUI} from "../../../testing/server-item-renderer-with-debug-ui"; | ||
| import { | ||
| incorrectAnswerQuestion, | ||
| mathQuestion, | ||
| shortTextQuestion, | ||
| textQuestion, | ||
| } from "../__tests__/label-image.testdata"; | ||
|
|
||
| import type {Meta} from "@storybook/react-vite"; | ||
|
|
||
| const meta: Meta = { | ||
| title: "Widgets/Label Image/Visual Regression Tests/Interactions", | ||
| component: ServerItemRendererWithDebugUI, | ||
| tags: ["!autodocs"], | ||
| parameters: { | ||
| chromatic: {disableSnapshot: false, modes: themeModes}, | ||
| }, | ||
| }; | ||
|
|
||
| export default meta; | ||
|
|
||
| // Verifies the marker open/selected state: clicking a marker button opens the | ||
| // answer choices dropdown and shows the active marker styling. | ||
| export const MarkerOpened = { | ||
| args: { | ||
| item: generateTestPerseusItem({question: textQuestion}), | ||
| }, | ||
| play: async ({canvasElement, userEvent}) => { | ||
| const canvas = within(canvasElement); | ||
| const marker = canvas.getByLabelText("The fourth unlabeled bar line."); | ||
| await userEvent.click(marker); | ||
| }, | ||
| }; | ||
|
|
||
| // Verifies the post-interaction marker state: after selecting an answer and | ||
| // closing the dropdown, all markers render as white circles (not the default | ||
| // pulsing blue). | ||
| export const AnswerSelected = { | ||
| args: { | ||
| item: generateTestPerseusItem({question: textQuestion}), | ||
| }, | ||
| play: async ({canvasElement, userEvent}) => { | ||
| const canvas = within(canvasElement); | ||
| const marker = canvas.getByLabelText("The fourth unlabeled bar line."); | ||
| await userEvent.click(marker); | ||
|
|
||
| // WonderBlocks SingleSelect renders options into a React portal outside | ||
| // the canvas, so we scope to document.body. | ||
| const suvsChoice = within(document.body).getByRole("option", { | ||
| name: "SUVs", | ||
| }); | ||
| await userEvent.click(suvsChoice); | ||
| }, | ||
| }; | ||
|
|
||
| // Verifies the correct answer state: after selecting the right answer and | ||
| // clicking Check, the marker and answer pill render in green (success.strong). | ||
| // Uses shortTextQuestion (single marker) to avoid needing to fill all markers. | ||
| // Check is clicked twice due to a server-side scoring quirk in Storybook. | ||
| export const CorrectAnswerGraded = { | ||
| args: { | ||
| item: generateTestPerseusItem({question: shortTextQuestion}), | ||
| }, | ||
| play: async ({canvasElement, userEvent}) => { | ||
| const canvas = within(canvasElement); | ||
|
|
||
| const marker = canvas.getByLabelText("The fourth unlabeled bar line."); | ||
| await userEvent.click(marker); | ||
|
|
||
| // WonderBlocks SingleSelect renders options into a React portal outside | ||
| // the canvas, so we scope to document.body. | ||
| const suvsChoice = within(document.body).getByRole("option", { | ||
| name: "SUVs", | ||
| }); | ||
| await userEvent.click(suvsChoice); | ||
|
|
||
| // eslint-disable-next-line testing-library/prefer-screen-queries | ||
| const checkButton = canvas.getByRole("button", {name: "Check answer"}); | ||
| await userEvent.click(checkButton); | ||
| await userEvent.click(checkButton); | ||
| }, | ||
| }; | ||
|
|
||
| // Verifies the incorrect answer state: marker dot renders with neutral | ||
| // background (background.neutral.default) and the answer pill shows the wrong | ||
| // selection with the same neutral styling. Uses incorrectAnswerQuestion, which | ||
| // has showCorrectness "incorrect" pre-set on the marker in the question data, | ||
| // matching how this state is passed in from review/show-solutions contexts. | ||
| // The play function selects an answer so the pill becomes visible. | ||
| export const IncorrectAnswerWithPill = { | ||
| args: { | ||
| item: generateTestPerseusItem({question: incorrectAnswerQuestion}), | ||
| }, | ||
| play: async ({canvasElement, userEvent}) => { | ||
| const canvas = within(canvasElement); | ||
|
|
||
| const marker = canvas.getByLabelText("The fourth unlabeled bar line."); | ||
| await userEvent.click(marker); | ||
|
|
||
| // WonderBlocks SingleSelect renders options into a React portal outside | ||
| // the canvas, so we scope to document.body. | ||
| const trucksChoice = within(document.body).getByRole("option", { | ||
| name: "Trucks", | ||
| }); | ||
| await userEvent.click(trucksChoice); | ||
| }, | ||
| }; | ||
|
|
||
| // Verifies that math choices render correctly inside an open marker dropdown. | ||
| // The math choices are only visible after opening a marker, so we capture | ||
| // the open state here. | ||
| export const MathChoicesVisible = { | ||
| args: { | ||
| item: generateTestPerseusItem({question: mathQuestion}), | ||
| }, | ||
| play: async ({canvasElement, userEvent}) => { | ||
| const canvas = within(canvasElement); | ||
| const marker = canvas.getByLabelText("The fourth unlabeled bar line."); | ||
| await userEvent.click(marker); | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| import {scoreLabelImageMarker} from "@khanacademy/perseus-score"; | ||
| import Clickable from "@khanacademy/wonder-blocks-clickable"; | ||
| import {View} from "@khanacademy/wonder-blocks-core"; | ||
| import {semanticColor} from "@khanacademy/wonder-blocks-tokens"; | ||
| import {StyleSheet, css} from "aphrodite"; | ||
| import classNames from "classnames"; | ||
| import * as React from "react"; | ||
|
|
@@ -862,7 +863,7 @@ const styles = StyleSheet.create({ | |
| marginLeft: 5, | ||
| marginRight: 5, | ||
|
|
||
| background: "rgba(33, 36, 44, 0.32)", | ||
| background: semanticColor.core.border.neutral.default, | ||
|
Comment on lines
863
to
+866
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The separator dot pseudo-element in Extended reasoning...Wrong semantic token category used for separator dot backgroundWhat the bug is: In The specific code path: The Why existing code does not prevent this: There is no type-level enforcement preventing a border token from being passed to a 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:
All three neutral-colored filled elements use Fix: Replace |
||
|
|
||
| borderRadius: 2, | ||
| }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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.strongAll 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-conversionand 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/perseuswill 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.