-
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-1847] Dropdown: Update SelectOpener
to match Design specs.
#2438
Conversation
…nternal component) from `Dropdown` to match Design specs. Also converts `color` tokens to use `semanticColor` tokens.
🦋 Changeset detectedLatest commit: b749fa9 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 |
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 (7980324) 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="PR2438" Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2438 |
Size Change: +170 B (+0.17%) Total Size: 97.8 kB
ℹ️ View Unchanged
|
This is the remaining work to replace this PR: #2422 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-wsroulwyvh.chromatic.com/ Chromatic results:
|
borderWidth: tokens.border.width.thin, | ||
paddingLeft: adjustedPaddingLeft, | ||
paddingRight: adjustedPaddingRight, | ||
const pressStyling = { |
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.
@marcysutton I've updated the variable name to match the new convention: press
.
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.
Oh nice, that looks consistent with the convention of state + "Styling"! The usage I was referencing in my prior comment is actually down at the bottom of the file, where pressStyling
is used: press: pressStyling
. The press
part is what I was curious about, since it differs from disabled
in how we reference state!
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 I see! press
is a new naming convention that we are using with Design. Wonder Blocks and Thunder Blocks are going to be updated soon to reflect that change as well to make it a bit more platform agnostic and match the other pseudo-states we currently define (default/rest
, hover
, focus
). As for disabled
, we are keeping it that way to keep it consistent with the disabled
/aria-disabled
HTML/ARIA attributes.
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.
Left some comments on semantic token usage! Visual diffs look good, the comments you left in Chromatic were very helpful in reviewing, thank you!
borderWidth: tokens.border.width.thin, | ||
paddingLeft: adjustedPaddingLeft, | ||
paddingRight: adjustedPaddingRight, | ||
outlineColor: error |
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.
Would this use the global outline semantic color token later on?
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.
That's correct! but for now we keep it separately as we want to continue using this approach in the current experience.
outlineOffset: -border.width.thin, | ||
outlineStyle: "solid", | ||
outlineWidth: border.width.thin, |
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 later): I wonder if we could use tokens for these values too! That would be an alternative way to share focus outline styles
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.
do you mean something like semantic tokens? or global tokens?
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 I see, it would be global semantic tokens. Yeah, the short term goal will be this, but this will happen after completing the color migration.
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.
Yes! Global semantic tokens for the focus outline styles!
? semanticColor.text.secondary | ||
: semanticColor.action.outlined.progressive.press.foreground | ||
: semanticColor.text.primary, | ||
outlineColor: error |
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.
Would this also use the global focus outline token?
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.
Ideally, yes! we'll have to see how the design specs are landed, but that's what we decided when we spoke about it.
? semanticColor.status.critical.background | ||
: semanticColor.surface.primary, | ||
borderColor: error | ||
? semanticColor.status.critical.foreground |
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 this be semanticColor.status.critical.border
?
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.
color: placeholder | ||
? tokens.color.offBlack64 | ||
: tokens.color.offBlack, | ||
? semanticColor.text.secondary |
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.
I wonder if it would be worth having a separate semantic text color for placeholder
. I'm updating the secondary text color to be fadedOffBlack72 for the subtitle in the Cell component. It's also currently used for the LabeledField description. It might be too dark for placeholder text!
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.
Good point! I raised that point in Figma and we are good to go with this change (using 72 for placeholders).
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.
Thanks for confirming!
borderWidth: tokens.border.width.hairline, | ||
paddingLeft: tokens.spacing.medium_16, | ||
paddingRight: tokens.spacing.small_12, | ||
? semanticColor.status.critical.foreground |
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 this be semanticColor.status.critical.border
?
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.
Same answer as the previous comment. To clarify this part a bit more. If we use status
, it border
and color
will share foreground
(most likely), and if we use action
, then we can use separate tokens.
background: tokens.color.offWhite, | ||
borderColor: tokens.color.offBlack16, | ||
color: tokens.color.offBlack64, | ||
background: semanticColor.action.disabled.secondary, |
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 this be semanticColor.action.disabled.background
?
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.
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.
No worries, we're also in a transition phase so lots of different things to consider! My questions are mostly around if the semantic meanings apply for the use cases, thanks for answering my questions! 😄
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! 🚀
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2438 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
Summary:
SelectOpener
to match design specs.color
tokens to usesemanticColor
tokens.Figma: https://www.figma.com/design/VbVu3h2BpBhH80niq101MHHE/%F0%9F%92%A0-Main-Components?node-id=13693-11133&t=W16iu9a1X5vqz5ez-4
NOTE: The
light
version was removed on a separate PR, so those design specs are not included here anymore.Issue: https://khanacademy.atlassian.net/browse/WB-1847
Test plan:
Verify that the
SelectOpener
component looks as expected in the All Variantsstories.