Update tooltip colors #50792
Merged
Update tooltip colors #50792
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -33 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6f74e13. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5047627913
|
|
It would be helpful with feedback on this one from component container contributors. |
|
My preference would likely be dictated by feedback around theming. In that respect I suppose it's a conceptual question about whether tooltips should be a shade lighter or darker than the base background color. In the examples below the lighter shade works a bit better for me. Both in terms of perceived elevation, and because the darker shade has a 'disabled' feel about it in light mode. What do you think? 
Of course Tooltips may be a unique case, and display the same regardless of mode / theme. I'll cc @SaxonF as I know he's thought about these details more than me :) |
mirka
reviewed
May 22, 2023
There was a problem hiding this comment.
Basically, the theme-ready gray-* scale is set up to darken in inverse directions based on whether the theme background is considered dark or light. (See this color scheme demo in Storybook and tweak the control for the background prop.)
Based on the theme-ready colors we have at our disposal right now, a straightforward combination would be:
background: $components-color-gray-100;
color: $components-color-foreground;This way the tooltip background is guaranteed to be at least slightly different from the themed background color. However, this will be a major stylistic change for all the current usages of Tooltip, being that most app backgrounds tend to be white.
CleanShot.2023-05-22.at.23.25.19.mp4
Tooltips may be a unique case, and display the same regardless of mode / theme.
Yes, a completely static setup (as proposed in this PR) is also probably fine for now, though not 100% theme-ready because we can't guarantee that black has sufficient contrast against the themed background color. But I am ok with merging as is, we can deal with it later when it comes to it 👍
|
Yeah the theming part is a bit more involved. I was playing with some ideas at the weekend and figured we may need to consider things like elevation here too. Not necessarily shadows, but the general order/layering of different materials (tooltips, popovers, snackbars, modals, the frame etc), and the affect that has on coloring. With that said, I'll merge this as it addresses a pain point in trunk, and we can circle back when we have a more robust color system in place. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Update the tooltip colors.
Why?
The current colorisation causes them to blend into the dark material in the site editor:
This PR changes the background color to
$gray-800, but black could work too:cc @ciampo @mirka as I noticed some preparatory theming styles were used here. I didn't want to update
$components-color-foregroundas I'm sure it's used elsewhere without issue.