-
Notifications
You must be signed in to change notification settings - Fork 10
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
[WB-1849] Rework action semantic color tokens #2428
Conversation
…tions to use a new structure. Every `action` now includes `default`, `hover` and `press` states, and each state includes `border`, `background` and `foreground` tokens.
🦋 Changeset detectedLatest commit: 480a8d1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 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 |
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 (6932af2) 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="PR2428" Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2428 |
Size Change: +167 B (+0.17%) Total Size: 98.5 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-xynstbxfft.chromatic.com/ Chromatic results:
|
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.
border: color.activeBlue, | ||
background: color.activeBlue, | ||
foreground: color.white, | ||
}, |
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.
Should the focus
state also have tokens associated with it?
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.
focus
will have its own separate state, that is directly allocated in the semanticColor.border.focus
token. This is something Caitlyn and I have discussed and the goal is that any focus on any component should use a blue outline, so that's the reason why I'm not including it here.
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.
Sounds good! There is also the disabled focus outline!
border: color.activeRed, | ||
background: color.fadedRed, | ||
foreground: color.activeRed, | ||
}, | ||
}, | ||
disabled: { |
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.
Should the tokens associated with the disabled
state also have the backgorund, foreground, border structure?
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.
It might be in the future, but for now I'm leaving them out of every specific semantic global token and I'm defining them inside Button
for now. Buttons need simplification and before doing that, I prefer to keep it this way. FWIW Design also has this structure, but this could change in the next weeks.
default: color.blue, | ||
active: color.activeBlue, | ||
// For primary actions | ||
progressive: { |
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.
For my own understanding, what was the reason for using the progressive
/progressiveInverse
terminology instead of primary
/secondary
?
And should we start using this terminology for component kind
variants too?
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.
ahh good question. progressive
refers to the type of action (defined as color
in Button
), which currently maps to blue
, and destructive
that maps to any action meant to delete/cancel etc (maps to red
).
The primary
and secondary
are more distinctive for the kind of look and feel.
I'd say that we can categorize these kinds this way:
primary
: Filled buttons (or buttons with a dark background).secondary
: outlined buttons (or buttons with white/transparent background).tertiary
: text buttons.
Now I'm wondering if we should create a third subcategory to account for the tertiary
case.
TBH our variants became overcomplicated at some point, and it would be nice to simplify this. I'm glad that design is trying to do it with the Polaris/ThunderBlocks theme.
@beaesguerra I made some changes based on your feedback/questions (also after discussing offline).
https://5e1bf4b385e3fb0020b7073c-xynstbxfft.chromatic.com/?path=/docs/foundations-using-color--docs Let me know what you think, and thanks again for the suggestions! |
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.
Looks great! Thanks for making those updates! 🎉
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2428 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
Summary:
Reworked
semanticColor
actions to use a new structure. Everyaction
nowincludes
default
,hover
andpress
states, and each state includesborder
,background
andforeground
tokens.By doing this change, we'll offer a more consistent and predictable styling
direction for our interactive components.
Issue: https://khanacademy.atlassian.net/browse/WB-1849
Test plan:
Navigate to
/?path=/docs/foundations-using-color--docs
and check that theAction
section includes all the new tokens and the structure is readable andmakes sense.