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.

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

Theme: Set color on wrapper #58095

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Theme: Set color on wrapper #58095

merged 2 commits into from
Jan 23, 2024

Conversation

mirka
Copy link
Member

@mirka mirka commented Jan 22, 2024

Part of #44116

What?

Set the themed foreground color as the color for the wrapper div on the Theme component.

Why?

While working on #58014, I noticed that the Theme component does not set the foreground color as a color for its wrapper div.

How?

Sets currentColor as the fallback when --wp-components-color-foreground is not defined. This can happen when the consumer has not provided a background prop on Theme.

I briefly considered falling back to GRAY[ 900 ], but that would probably be a bit too intrusive 🤔 We can revisit this if necessary.

Testing Instructions

  1. Look at a Storybook story with some text that is not explicitly colored, e.g. the View component.

  2. Using the Theme switcher in the toolbar, try out the different presets.

    • For the two presets that set a background color, the text color should responsively change to either #ffffff or #1e1e1e depending on the background.
    • In the "Classic" preset that only sets an accent color, the text color will stay as #000000 (= currentColor).
    • In trunk, the text colors won't change at all.
CleanShot.2024-01-23.at.06.45.17.mp4

@mirka mirka added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Jan 22, 2024
@mirka mirka self-assigned this Jan 22, 2024
@mirka mirka requested a review from ajitbohra as a code owner January 22, 2024 21:48
@mirka mirka requested a review from a team January 22, 2024 21:50
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I briefly considered falling back to GRAY[ 900 ], but that would probably be a bit too intrusive 🤔 We can revisit this if necessary.

Agreed, this PR represents an improvement over what's on trunk anyway,

@mirka mirka merged commit 982a517 into trunk Jan 23, 2024
57 checks passed
@mirka mirka deleted the add/theme-color branch January 23, 2024 18:37
@github-actions github-actions bot added this to the Gutenberg 17.6 milestone Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants