[WB-1816] Rework semanticColor tokens to be exported as CSS variables
#2481
Conversation
…emantic color tokens as css variables
…ge themes using CSS variables
…h references to css variables. Creates a build step to convert semanticColor tokens to css variables that can be consumed via CSS imports.
🦋 Changeset detectedLatest commit: 5f147af The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 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: +273 B (+0.28%) Total Size: 98.2 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-fhkfqaiwix.chromatic.com/ Chromatic results:
|
There was a problem hiding this comment.
note: I'm just adding some arbitrary values here for testing purposes (and to show how it's going to work), but I'll remove this before landing this PR.
There was a problem hiding this comment.
note: I just moved all the contents from tokens/semantic-color.ts to this file.
There was a problem hiding this comment.
note: This is one of the main changes:
- Instead of exporting a static object, I'm exporting the result of the function that iterates over the default semanticColor object and replaces all values to references to CSS variables.
- The other part of the change is made at the build process, where we generate the
styles.cssfile indistto export all the semantic-color tokens as css variables.
| "exports": { | ||
| ".": { | ||
| "import": "./dist/es/index.js", | ||
| "require": "./dist/index.js" | ||
| }, | ||
| "./styles.css": "./dist/css/index.css" | ||
| }, |
There was a problem hiding this comment.
note: This change is required for the consumers to be able to import/resolve the CSS file correctly.
This means that consumers can do this:
import "@khanacademy/wonder-blocks-tokens/styles.css";
| @@ -1,4 +1,3 @@ | |||
| import * as tokens from "@khanacademy/wonder-blocks-tokens"; | |||
There was a problem hiding this comment.
note: Changed this to avoid a circular dependencies issue between theming and tokens. (we only use tokens in this package for testing purposes).
| : // NOTE(WB-1880): This will be simplified once we split this into Badge and Pill. | ||
| `color-mix(in srgb, ${tokens.color.offBlack32}, ${backgroundColor})`; |
There was a problem hiding this comment.
note: I applied this change to prevent an issue with mix() when used with css variables. This change should be safe for two reasons:
- These variants are not currently used in
webapp. - We are going to be able to use
color-mix()very soon (according to our upcoming browser support matrix).
| @@ -3,6 +3,7 @@ import Choice from "./components/choice"; | |||
| import CheckboxGroup from "./components/checkbox-group"; | |||
| import RadioGroup from "./components/radio-group"; | |||
| import TextField from "./components/text-field"; | |||
| // eslint-disable-next-line import/no-deprecated | |||
There was a problem hiding this comment.
note: ESLint complained about this, so I added these lines in this file.
There was a problem hiding this comment.
Ohh interesting, I recently added the @deprecated tag for LabeledTextField, but didn't get any lint errors in the PR checks 🤔
There was a problem hiding this comment.
I think I know why! The PR lint check only linted the files changed in the PR! Thanks for fixing it here!
https://github.com/Khan/wonder-blocks/actions/runs/13462088339/job/37619603166?pr=2476
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (6c89458) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2481"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2481 |
| // NOTE: External consumers should import the CSS variables from the | ||
| // wonder-blocks-tokens package directly. | ||
| // e.g. import "@khanacademy/wonder-blocks-tokens/styles.css"; | ||
| import "../node_modules/@khanacademy/wonder-blocks-tokens/dist/css/index.css"; |
There was a problem hiding this comment.
question: Is there a reason why you can't import the styles.css file from the tokens like you'd recommend for consumers?
There was a problem hiding this comment.
For some reason, Storybook wasn't able to resolve that import here. It's probably down to some vite setting that I'm missing. I tried to add the wb-tokens import directly in the root package.json but that didn't solve the issue.
| @@ -388,7 +388,7 @@ const styles = StyleSheet.create({ | |||
| }, | |||
| ":focus-visible": { | |||
| // TODO(WB-1856): Verify if we can use the global focus color | |||
There was a problem hiding this comment.
Can this TODO comment be removed with this PR, or is that different work?
There was a problem hiding this comment.
This is happening here: #2480
| : // NOTE(WB-1880): This will be simplified once we split this into Badge and Pill. | ||
| mix(tokens.color.offBlack32, backgroundColor); | ||
| kind === "transparent" || kind === "neutral" | ||
| ? tokens.color.offBlack16 |
There was a problem hiding this comment.
I'm surprised to see semanticColor replaced by tokens.color. Is that intentional?
There was a problem hiding this comment.
ahhh hmmm yeah, so turned out that I used the incorrect semantic token (compared to the Figma specs), and we don't have that value represented as a semantic token, so I went back to using a primitive for this case for now. I'll revisit this as soon as we split this component + add theming support to it. I'm going to add a TODO comment to provide better visibility of when this is going to be fixed.
| // Remove the file extension | ||
| const filename = file.split(".")[0]; | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const {default: themeObject} = require(`../theme/color/${filename}`); |
There was a problem hiding this comment.
Can you use THEMES_DIR here to make it easy to update?
const {default: themeObject} = require(`${THEMES_DIR}/${filename}`);There was a problem hiding this comment.
Good catch! I'll replace it :)
| // Import the Wonder Blocks CSS variables | ||
| // NOTE: External consumers should import the CSS variables from the | ||
| // wonder-blocks-tokens package directly. | ||
| // e.g. import "@khanacademy/wonder-blocks-tokens/styles.css"; |
There was a problem hiding this comment.
Do we have instructions for setting up WB in projects somewhere? We could add these details there too! I poked around and couldn't find any docs for setting up though! (This could be a later task 😄 also happy to take this on, if we think it would be useful!)
There was a problem hiding this comment.
Great question! we don't have explicit instructions, and that would be great. Go for it if you could work on those docs :)
There was a problem hiding this comment.
Created WB-1892 for set up docs! 😄
.storybook/preview.tsx
Outdated
| { | ||
| value: "classroom", | ||
| icon: "lightning", | ||
| title: "Classroom", |
There was a problem hiding this comment.
Do we want to name this Thunder Blocks since that's the name of the associated Figma library? Or Classroom (Thunder Blocks) or Thunder Blocks (Classroom)?
There was a problem hiding this comment.
sounds good! I'll go with Thunder Blocks (Classroom).
| "@khanacademy/wonder-blocks-link": "workspace:*", | ||
| "@khanacademy/wonder-blocks-tokens": "workspace:*", | ||
| "@khanacademy/wonder-blocks-theming": "workspace:*", |
There was a problem hiding this comment.
For my own learning, how do we know when we should include WB packages as a dev dependency at the root level?
There was a problem hiding this comment.
For some context on why I included this here: this was supposed to fix the SB issue not properly resolving the styles.css import from tokens. The other imports were added as we are using them in .storybook/preview.tsx.
As for devDeps vs regular deps, I think the distinction mostly applies for nested package.json files (all the WB packages), but for the root package, I think we should include them as devDep as these are packages used/required by internal tooling (Storybook in this case).
There was a problem hiding this comment.
Makes sense, thanks for the explanation!
| @@ -3,6 +3,7 @@ import Choice from "./components/choice"; | |||
| import CheckboxGroup from "./components/checkbox-group"; | |||
| import RadioGroup from "./components/radio-group"; | |||
| import TextField from "./components/text-field"; | |||
| // eslint-disable-next-line import/no-deprecated | |||
There was a problem hiding this comment.
Ohh interesting, I recently added the @deprecated tag for LabeledTextField, but didn't get any lint errors in the PR checks 🤔
| * NOTE: The prefix uses the following structure: | ||
| * --wb-<namespace>-<token_category>- | ||
| * | ||
| * - namespace: The design token namespace (`s` for semantic tokens). |
There was a problem hiding this comment.
Related to namespaces, would p be the other namespace for primitive tokens? Would primitive tokens also use css variables?
There was a problem hiding this comment.
yep! p stands for primitives. I'm still not sure if primitive tokens will be exported as CSS variables. I'm trying to be very explicit on what's meant to be themed and want to encourage folks to use semantic variables, so I'm not sure if exposing primitives as CSS vars would give the right signal. Would love to hear your thoughts about this!
There was a problem hiding this comment.
I agree, I don't think we need to expose the primitive colors as css vars (for now at least!)
package.json
Outdated
| @@ -25,7 +26,7 @@ | |||
| "lint:ci": "eslint --ext .ts --ext .js --ext .tsx --ext .jsx", | |||
| "publish:check": "pnpm run lint:pkg-json && ./utils/publish/pre-publish-check-ci.ts", | |||
| "publish:ci": "pnpm run publish:check && git diff --stat --exit-code HEAD && pnpm build && pnpm build:types && pnpm changeset publish", | |||
| "start": "pnpm start:storybook", | |||
| "start": "pnpm dev & pnpm start:storybook", | |||
There was a problem hiding this comment.
What do you think about moving pnpm dev to the start:storybook script instead so it'll always run with storybook? (I ask because I checked out the branch locally to poke around and pnpm start:storybook wasn't working because it couldn't find the new css file!)
| title: "Khanmigo", | ||
| }, | ||
| { | ||
| value: "classroom", |
There was a problem hiding this comment.
Going forward, how would we like to account for theming in our stories for Chromatic tests? Include all themes in the All Variants stories? Or all types of stories? Curious to hear what others think!
There was a problem hiding this comment.
I could see two ways for tackling this:
-
I know that Chromatic supports modes for these cases. In theory, this looks like a great take on this! Unfortunately, this would mean that the # of snapshots would be multiplied and that's a big reason why I'd be hesitant to use this approach.
-
Including themes in the
All variantsstories. Which is what we have been using for the existingkhanmigotheme. The story/snapshot would be twice the size, but at least we would have the same number of snapshots being sent to Chromatic.
My vote would be #2 for trying to keep the chromatic snapshots number down.
What do you all think? Is there another approach?
There was a problem hiding this comment.
I agree, #2 makes sense to keep snapshots number down! I think as long as the All Variants story cover the main use cases, we'll get good coverage of the same features across different themes!
Summary:
semanticColorvalues with references to css variables.Also, adds a build step to convert
semanticColortokens to css variables thatcan be consumed via CSS imports.
ThemeSwitchercomponent to change themes using CSSvariables.
mix()withcolor-mix()for active background colors.Issue: WB-1816
Test plan:
/?path=/story/packages-labeledfield--validation.Themedropdown in the storybook toolbar.Classroom).Screen.Recording.2025-02-27.at.1.04.41.PM.mov