-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
DropdownMenu V2: use themed color variables #64647
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
const DEFAULT_BORDER_COLOR = COLORS.gray[ 300 ]; | ||
const DIVIDER_COLOR = COLORS.gray[ 200 ]; | ||
const TOOLBAR_VARIANT_BORDER_COLOR = COLORS.gray[ '900' ]; | ||
// - border color and divider color are different from COLORS.theme variables |
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.
Opened #64649 to discuss this separately
// - lighter text color is not defined in COLORS.theme, should it be? | ||
// - lighter background color is not defined in COLORS.theme, should it be? |
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.
Maybe these two questions are trivial enough that I can add those variables directly in the PR, if folks agree.
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 have a feeling we shouldn't be doing too much abstracting or "semanticizing" right now, and rather wait until the design folks are ready with a new token system. I'm assuming this color system in the Figma is still WIP, but in any case the token system there is very different from the one we currently have in @wordpress/components
.
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's definitely true that the priority is to make such progress directly on the theme effort, which will then represent the source of truth for those design tokens.
I thought we could have a temporary internal set of variables to help with consistency and with the refactor to the new design tokens, but I'm also completely fine leaving the code as-is.
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 good 🚀
2f89729
to
d9d4ff0
Compare
* Use foreground theme color instead of hardcoded gray 900 * Use shared theme variable for gray 700 text * Use theme variables for remaining colors.gray variables * Update comments * Extract lighter background color * CHANGELOG --- Co-authored-by: ciampo <[email protected]> Co-authored-by: mirka <[email protected]>
What?
Related to #50971
Update
DropdownMenuV2
styles to use as much as possible themed color variables, instead of hardcoded onesWhy?
Aiming for greater consistency across our UIs by having shared colors and codified guidelines.
How?
Updating styles in
DropdownMenu
v2Testing Instructions
Load storybook, make sure that the dropdown menu v2 examples keep looking as before
Note
Any theming applied in Storybook (via the dedicated top toolbar menu) won't apply to the dropdown menu popover, since it's portaled in a part of the page that doesn't have the theme variables in scope (cc @mirka )