-
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
Components: Fixing Text Contrast for Dark Mode #68349
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. |
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 and works great! 👍 ✅
I've fixed a few snapshot and unit tests that needed to be updated, and will ship it shortly. 🚢
Thanks for your contribution @im3dabasia 🙌
? css( { color: COLORS.theme.foreground } ) | ||
: css( { color: COLORS.theme.foregroundInverted } ); |
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.
Since this is the logic for the optimizeReadabilityFor
prop that takes a static background color as input, these text colors need to remain as non-themed values.
For example, when <Text optimizeReadabilityFor="#000">
the text color needs to be white, regardless of the theme colors.
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.
Thank you @mirka
The previous code on the trunk had static color strings returned, but these are deprecated. As per a comment, it is recommended to switch to theme-ready variables from COLORS.theme.
Previously, the trunk contained the following code before this merge:
Code before merge
if ( optimizeReadabilityFor ) {
const isOptimalTextColorDark =
getOptimalTextShade( optimizeReadabilityFor ) === 'dark';
sx.optimalTextColor = isOptimalTextColorDark
? css( { color: COLORS.gray[ 900 ] } )
: css( { color: COLORS.white } );
}
I encounter deprecation warnings when using COLORS.white
or COLORS.gray[900]
.
Code
gutenberg/packages/components/src/utils/colors-values.js
Lines 44 to 54 in e5dca54
gray: { | |
/** @deprecated Use `COLORS.theme.foreground` instead. */ | |
900: `var(--wp-components-color-foreground, ${ GRAY[ 900 ] })`, | |
800: `var(--wp-components-color-gray-800, ${ GRAY[ 800 ] })`, | |
700: `var(--wp-components-color-gray-700, ${ GRAY[ 700 ] })`, | |
600: `var(--wp-components-color-gray-600, ${ GRAY[ 600 ] })`, | |
400: `var(--wp-components-color-gray-400, ${ GRAY[ 400 ] })`, | |
300: `var(--wp-components-color-gray-300, ${ GRAY[ 300 ] })`, | |
200: `var(--wp-components-color-gray-200, ${ GRAY[ 200 ] })`, | |
100: `var(--wp-components-color-gray-100, ${ GRAY[ 100 ] })`, | |
}, |
gutenberg/packages/components/src/utils/colors-values.js
Lines 78 to 81 in e5dca54
/** | |
* @deprecated Prefer theme-ready variables in `COLORS.theme`. | |
*/ | |
white, |
Would reverting these changes be a good approach to address this? I can create a PR if needed, but I’d like to hear your thoughts first.
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 reverting these changes be a good approach to address this?
Yes. I feel like we might deprecate the optimizeReadabilityFor
prop once theming support has stabilized, but for now I'm fine with keeping the deprecated variables here. Maybe with a code comment like:
// Should not use theme colors
sx.optimalTextColor = isOptimalTextColorDark
? css( { color: COLORS.gray[ 900 ] } )
: css( { color: COLORS.white } );
Part of: #66454
Comment: #66454 (comment)
Work in progress
What?
Fixes the components labels/headings in the dark mode
Testing Instructions
Screenshots or screencast
AnglePickerControl
CustomGradientPicker
FocalPointPicker
GradientPicker
InputControl
NumberControl
PaletteEdit
QueryControl
SelectControl
TreeSelect
UnitControl