Skip to content
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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Dec 30, 2024
Merged

Components: Fixing Text Contrast for Dark Mode #68349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4868ee3
fix: Fix darkmode label for text component
im3dabasia Dec 27, 2024
74a8363
fix: darkmode for heading component
im3dabasia Dec 27, 2024
92c87cc
Fix snapshot tests
tyxla Dec 30, 2024
7bc096e
Fix Text test
tyxla Dec 30, 2024
f44ff04
CHANGELOG
tyxla Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

- `BoxControl`: Add presets support ([#67688](https://github.com/WordPress/gutenberg/pull/67688)).
- `Navigation`: Upsize back buttons ([#68157](https://github.com/WordPress/gutenberg/pull/68157)).
- `Heading`: Fix text contrast for dark mode ([#68349](https://github.com/WordPress/gutenberg/pull/68349)).
- `Text`: Fix text contrast for dark mode ([#68349](https://github.com/WordPress/gutenberg/pull/68349)).

### Deprecations

Expand Down
8 changes: 4 additions & 4 deletions packages/components/src/dimension-control/test/__snapshots__/index.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ exports[`DimensionControl rendering renders with custom sizes 1`] = `
}

.emotion-12 {
color: #1e1e1e;
color: var(--wp-components-color-foreground, #1e1e1e);
line-height: 1.4;
margin: 0;
text-wrap: balance;
Expand Down Expand Up @@ -345,7 +345,7 @@ exports[`DimensionControl rendering renders with defaults 1`] = `
}

.emotion-12 {
color: #1e1e1e;
color: var(--wp-components-color-foreground, #1e1e1e);
line-height: 1.4;
margin: 0;
text-wrap: balance;
Expand Down Expand Up @@ -637,7 +637,7 @@ exports[`DimensionControl rendering renders with icon and custom icon label 1`]
}

.emotion-12 {
color: #1e1e1e;
color: var(--wp-components-color-foreground, #1e1e1e);
line-height: 1.4;
margin: 0;
text-wrap: balance;
Expand Down Expand Up @@ -941,7 +941,7 @@ exports[`DimensionControl rendering renders with icon and default icon label 1`]
}

.emotion-12 {
color: #1e1e1e;
color: var(--wp-components-color-foreground, #1e1e1e);
line-height: 1.4;
margin: 0;
text-wrap: balance;
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/heading/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function useHeading(
const {
as: asProp,
level = 2,
color = COLORS.gray[ 900 ],
color = COLORS.theme.foreground,
isBlock = true,
weight = CONFIG.fontWeightHeading as import('react').CSSProperties[ 'fontWeight' ],
...otherProps
Expand Down
8 changes: 4 additions & 4 deletions packages/components/src/heading/test/__snapshots__/index.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@

exports[`props should render correctly 1`] = `
.emotion-0 {
color: #1e1e1e;
color: var(--wp-components-color-foreground, #1e1e1e);
line-height: 1.4;
margin: 0;
text-wrap: balance;
text-wrap: pretty;
color: #1e1e1e;
color: var(--wp-components-color-foreground, #1e1e1e);
font-size: calc(1.95 * 13px);
font-weight: 600;
display: block;
Expand All @@ -30,7 +30,7 @@ Snapshot Diff:
@@ -1,10 +1,10 @@
Array [
Object {
"color": "#1e1e1e",
"color": "var(--wp-components-color-foreground, #1e1e1e)",
"display": "block",
- "font-size": "calc(1.25 * 13px)",
+ "font-size": "calc(1.95 * 13px)",
Expand All @@ -49,7 +49,7 @@ Snapshot Diff:
@@ -1,10 +1,10 @@
Array [
Object {
"color": "#1e1e1e",
"color": "var(--wp-components-color-foreground, #1e1e1e)",
"display": "block",
- "font-size": "calc(1.25 * 13px)",
+ "font-size": "calc(1.95 * 13px)",
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/text/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ export default function useText(
getOptimalTextShade( optimizeReadabilityFor ) === 'dark';

sx.optimalTextColor = isOptimalTextColorDark
? css( { color: COLORS.gray[ 900 ] } )
: css( { color: COLORS.white } );
? css( { color: COLORS.theme.foreground } )
: css( { color: COLORS.theme.foregroundInverted } );
Comment on lines +108 to +109
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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 } );

}

return cx(
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/text/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { css } from '@emotion/react';
import { COLORS, CONFIG } from '../utils';

export const Text = css`
color: ${ COLORS.gray[ 900 ] };
color: ${ COLORS.theme.foreground };
line-height: ${ CONFIG.fontLineHeightBase };
margin: 0;
text-wrap: balance; /* Fallback for Safari. */
Expand Down
6 changes: 3 additions & 3 deletions packages/components/src/text/test/__snapshots__/index.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Snapshot Diff:
+ Base styles

@@ -3,8 +3,9 @@
"color": "#1e1e1e",
"color": "var(--wp-components-color-foreground, #1e1e1e)",
"font-size": "calc((13 / 13) * 13px)",
"font-weight": "normal",
"line-height": "1.4",
Expand All @@ -19,7 +19,7 @@ Snapshot Diff:

exports[`Text should render highlighted words with highlightCaseSensitive 1`] = `
.emotion-0 {
color: #1e1e1e;
color: var(--wp-components-color-foreground, #1e1e1e);
line-height: 1.4;
margin: 0;
text-wrap: balance;
Expand Down Expand Up @@ -52,7 +52,7 @@ exports[`Text should render highlighted words with highlightCaseSensitive 1`] =

exports[`Text snapshot tests should render correctly 1`] = `
.emotion-0 {
color: #1e1e1e;
color: var(--wp-components-color-foreground, #1e1e1e);
line-height: 1.4;
margin: 0;
text-wrap: balance;
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/text/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ describe( 'Text', () => {
</Text>
);
expect( screen.getByRole( 'heading' ) ).toHaveStyle( {
color: COLORS.white,
color: 'rgb( 255, 255, 255 )',
} );
} );

Expand Down
Loading