Use semantic tokens for CellCore #2448
Conversation
🦋 Changeset detectedLatest commit: e57290b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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: +151 B (+0.15%) Total Size: 98.5 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-hgakpygect.chromatic.com/ Chromatic results:
|
| const cellTokens = { | ||
| hover: { | ||
| background: color.fadedBlue8, | ||
| }, | ||
| press: { | ||
| background: color.fadedBlue8, | ||
| }, | ||
| selected: { | ||
| background: color.fadedBlue8, | ||
| foreground: color.activeBlue, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The semantic colors from our current action tokens don't apply to the cell:
- the cell hover and press background is different
- there is no selected state for actions currently
I've created this set of tokens for now so that we can swap it out later to support theming. All the usage of primitive colors in this component are consolidated to this object now! We could create new global semantic tokens for this, though I'm not sure if it would be useful for other things (this may be a one-off)
Let me know if there's something else I can do to help support the token work.
There was a problem hiding this comment.
This is great! thanks a lot. I agree with you that this probably makes sense to keep it as component tokens for now.
beaesguerra
force-pushed
the
cell-semantic-tokens
branch
from
1e63c34 to
0618986
Compare
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 (1b649a0) 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="PR2448"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2448 |
| const cellTokens = { | ||
| hover: { | ||
| background: color.fadedBlue8, | ||
| }, | ||
| press: { | ||
| background: color.fadedBlue8, | ||
| }, | ||
| selected: { | ||
| background: color.fadedBlue8, | ||
| foreground: color.activeBlue, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
This is great! thanks a lot. I agree with you that this probably makes sense to keep it as component tokens for now.
| background: semanticColor.surface.primary, | ||
| color: semanticColor.text.primary, |
There was a problem hiding this comment.
suggestion/thought: It's great that you created the cellTokens object. How do you feel about moving this to that object as I'm thinking this could go in the component theme object.
e.g.
const cellTokens = {
default: {
background: semanticColor.surface.primary,
foreground: semanticColor.text.primary,
},
...
};
There was a problem hiding this comment.
Updated! I added more to cellTokens. I was going to move all the semantic token usage to cellTokens, though I wasn't sure about other things like the color for the icon since I haven't seen that in the other PRs. Let me know though what you think!
There was a problem hiding this comment.
Great question! I'm curious to hear your thoughts on how to approach those tokens for the other elements in the component.
There was a problem hiding this comment.
If we wanted to include other elements in the component tokens, we could have something like this:
const cellTokens = {
default: {
background: semanticColor.surface.primary,
foreground: semanticColor.text.primary,
icon: semanticColor.icon.primary, // <--
},
selected: {
background: color.fadedBlue8,
foreground: color.activeBlue,
border: semanticColor.surface.emphasis,
icon: semanticColor.icon.action, // <--
},
};
I'm wondering now, what if there were multiple icons that are supposed to have different colors? Or maybe there's different text colors in the component? We could get more specific in the component tokens:
const componentTokens = {
default: {
background: semanticColor.surface.primary,
foreground: semanticColor.text.primary,
startIcon: semanticColor.icon.primary,
endIcon: semanticColor.icon.primary,
subtitle: semanticColor.text.secondary,
},
};
But then, what about other non-color properties, like font size or border radius? We could normalize it so the elements are at the first level (option A) so it doesn't get to nested (option B)
// Option A
const componentTokens = {
root: {
default: { background: ..., foreground: ... },
hover: { background: ..., foreground: ... },
...
},
leftIcon: {
default: { foreground: ..., height: ... },
hover: { foreground: ..., height: ... },
},
subtitle: {
default: { foreground: ..., fontSize: .... },
hover: { foreground: ..., fontSize: .... },
},
};
// Option B
const componentTokens = {
root: {
default: {
background: ...,
foreground: ...,
leftIcon: {
foreground: ...,
height: ...,
},
subtitle: {
foreground: ...,
fontSize: ....
}
},
hover: {
background: ...,
foreground: ...,
leftIcon: {
foreground: ...,
height: ...,
},
subtitle: {
foreground: ...,
fontSize: ....
}
},
...
},
};
Let me know what you think, happy to chat more about this!
There was a problem hiding this comment.
Thanks for your thinking on that! Both approaches are super valid, but I have a slight preference over A) as I think it makes easier to consume tokens dynamically. I feel that we currently don't have a defined structure for our component tokens (I mean every component is different), but maybe it is a good point to think about that.
There was a problem hiding this comment.
I updated the structure following option A! I moved all the semanticColor token usage to the cellTokens object. Let me know what you think!
…se is within cellTokens object
Summary:
Use semantic tokens for cell-core.
Issue: WB-1797
Test plan: