[WB-1814.10] Refactor wonder-blocks-dropdown to use semantic colors #2474
Conversation
🦋 Changeset detectedLatest commit: d71e95d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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: +433 B (+0.44%) Total Size: 98 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-ighzpzedjt.chromatic.com/ Chromatic results:
|
…emanticColor. Simplify ActionMenuOpenerCore styles
…ptionItem and ActionItem
| background: offBlack, | ||
| }, | ||
| color: theme.hover.foreground, | ||
| background: theme.hover.background, | ||
| }, |
There was a problem hiding this comment.
note: I removed this as it is no longer needed. I checked other popular design systems in the industry and all of them preserve the hover state on mobile devices, so we just let the browsers decide how to apply these styles from now on.
|
|
||
| const styles: Record<string, any> = {}; | ||
|
|
||
| const _generateStyles = (localColor: string) => { |
There was a problem hiding this comment.
note: This was overhead, so I simplified these styles by moving them to the StyleSheet definition above.
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 (c61daab) 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="PR2474"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2474 |
| props: {disabled: true}, | ||
| }, | ||
| { | ||
| name: "Custon label", |
There was a problem hiding this comment.
nit: typo!
| name: "Custon label", | |
| name: "Custom label", |
| (Story): React.ReactElement<React.ComponentProps<typeof View>> => ( | ||
| <View | ||
| style={{ | ||
| background: semanticColor.surface.secondary, |
There was a problem hiding this comment.
What do you think about using the Storybook background control to set the background instead? That way, if we need to, we can change the background color when testing to see how it looks with other backgrounds! https://storybook.js.org/docs/essentials/backgrounds
| hover: { | ||
| background: semanticColor.action.filled.progressive.hover.background, | ||
| foreground: semanticColor.action.filled.progressive.hover.foreground, | ||
| }, | ||
| press: { | ||
| background: semanticColor.action.filled.progressive.press.background, | ||
| foreground: semanticColor.action.filled.progressive.press.foreground, | ||
| }, |
There was a problem hiding this comment.
I would have thought that this would use the outlined tokens since in it's rest state, it looks more like the outlined styles than filled (it is still a bit different though)! How do we decide if we should use the filled or outlined styles?
There was a problem hiding this comment.
Ugh I know. This is one of these inconsistencies that we should address very soon.
| @@ -122,8 +119,13 @@ const sharedStyles = StyleSheet.create({ | |||
| WebkitTapHighlightColor: "rgba(0,0,0,0)", | |||
| }, | |||
| }, | |||
| default: { | |||
| background: "none", | |||
There was a problem hiding this comment.
Should a background of none be tokenized?
There was a problem hiding this comment.
Probably yes (will ask Caitlyn about this), but that's something that I'm going to address as soon as I add theming support to dropdowns. https://khanacademy.atlassian.net/browse/WB-1868
| disabled: { | ||
| cursor: "auto", | ||
| color: semanticColor.action.disabled.default, | ||
| cursor: "default", |
There was a problem hiding this comment.
Do we use the not-allowed cursor on disabled states?
| @@ -69,16 +67,16 @@ const styles = StyleSheet.create({ | |||
| borderRadius: 3, | |||
| borderWidth: 1, | |||
| borderStyle: "solid", | |||
| borderColor: offBlack50, | |||
| borderColor: semanticColor.border.strong, | |||
There was a problem hiding this comment.
Out of curiosity, should the Checkbox in the dropdown always have the same style as the Checkbox component from the form package? Would it make sense to share styles between the 2? Or let them each evolve on their own?
If they should always be the same styles, another alternative could be to use the Checkbox component from the form package directly!
There was a problem hiding this comment.
Good question! I think both need to be separate in terms of semantics and styling. This dropdown checkbox is not focusable/interactive, and instead it is more decorative in the sense that is meant to present a selected/unselected state in the current option item. Another difference is that the form one has a focused style (outline ring) where as this does not include that as the focus is set in the option item element.
| background: semanticColor.action.disabled.secondary, | ||
| border: `1px solid ${semanticColor.border.primary}`, | ||
| color: semanticColor.text.secondary, |
There was a problem hiding this comment.
Do you think it would be helpful to follow the background/foreground/outline structure for the primary and secondary disabled action tokens? So that the 3 tokens are always used together!
There was a problem hiding this comment.
Could you please elaborate on this? would it be to add disabled in outlined and filled?
FYI I moved these tokens for now to a theme object that I'll later move to a dropdown theme file.
There was a problem hiding this comment.
Sure! For other action colors, we have (for example)semanticColor.action.filled.progressive.default which has
- border
- background
- foreground
We define the 3 properties for other states and variants. I'm wondering if it would be helpful to have border, background, and foreground for the disabled actions too.
// Before (shows part of the semantic color token structure)
const action = {
disabled: {
default: color.fadedOffBlack32,
secondary: color.offWhite,
}
}
// After (default and secondary are expanded)
const action = {
disabled: {
default: {
background: color.fadedOffBlack32,
foreground: semanticColor.text.inverse,
border: semanticColor.border.primary,
},
secondary: {
background: color.offWhite,
foreground: semanticColor.text.secondary,
border: semanticColor.border.primary,
},
}
Then applying the disabled action tokens would look like:
const styles = {
disabled: {
background: semanticColor.action.disabled.secondary.background,
border: `1px solid ${semanticColor.action.disabled.secondary.border}`,
color: semanticColor.action.disabled.secondary.foreground,
}
}
Grouping the foreground, background and border together at the semantic tokens level could help make sure the correct values are used together so that they have enough color contrast (and for consistency)!
There was a problem hiding this comment.
Ohh I see, thanks! yeah that makes a lot of sense :)
In fact, Design is taking a similar approach. I'm going to discuss with Caitlyn how we are going to align with these tokens and add this change in a separate PR. I'll add a TODO comment for now.
There was a problem hiding this comment.
Here's the ticket: https://khanacademy.atlassian.net/browse/WB-1889
| }, | ||
| hover: { | ||
| background: | ||
| semanticColor.action.filled.progressive.hover.background, |
There was a problem hiding this comment.
Same question about filled vs outlined tokens!
| semanticColor.action.filled.progressive.press.foreground, | ||
| }, | ||
| disabled: { | ||
| background: "transparent", |
There was a problem hiding this comment.
Should we create a color token for transparent?
| foreground: semanticColor.text.disabled, | ||
| }, | ||
| }, | ||
| checkbox: { |
There was a problem hiding this comment.
Same question about if checkbox styles need to stay in sync with the Checkbox component from the form package!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2474 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
Summary:
Last PR that migrates the remaining components to use semantic colors.
All Variantsstories toActionItemandOptionItem.ActionItemandOptionItemto use semantic colors.ActionMenu,SingleSelect,MultiSelectandComboboxtouse semantic colors.
ActionMenuOpenerCoreto use a single staticStyleSheet instance (instead of generating another one in runtime).
Implementation plan:
Issue: WB-1814
Test plan:
Verify that all the dropdown stories are still working as expected.