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

Components: Update text colors #40113

Closed
mirka opened this issue Apr 6, 2022 · 5 comments · Fixed by #40391
Closed

Components: Update text colors #40113

mirka opened this issue Apr 6, 2022 · 5 comments · Fixed by #40391
Assignees
Labels
[Package] Components /packages/components

Comments

@mirka
Copy link
Member

mirka commented Apr 6, 2022

What problem does this address?

There are outdated (?) text colors in some components, such as:

  • Heading: Uses #050505, which is explicitly different from the default text color
  • InputControl: Uses #000000
  • SelectControl: Uses #000000

There are probably more.

What is your proposed solution?

Some possible strategies, in order of personal preference:

  • Remove the explicit color declaration. These default text colors should simply be inherited from the surrounding context instead of explicitly setting them in each component. (This may not be backwards compatible though.)
  • Start consolidating the colors to a CSS variable instead of a JS constant or Sass variable.
  • Change the values to darkGray.primary.

@ciampo Any thoughts?

@ciampo
Copy link
Contributor

ciampo commented Apr 6, 2022

Remove the explicit color declaration.

I don't think we can / want / should do this — as you say, there's a high potential for breaking changes and unexpected behaviors.

Change the values to darkGray.primary

This seems to be the correct short term solution, especially if we want to apply these changes in time for WP 6.0 — although probably good to get @jasmussen's (or other key stakeholders) blessing 🌟

Start consolidating the colors to a CSS variable instead of a JS constant or Sass variable.

I like this as it would also move us towards being ready to expose a lightweight theming API. I think this should be considered as our medium-term solution, as it may also require some refactor / tidy-up in the internal config.

WDYT ?

@mirka
Copy link
Member Author

mirka commented Apr 6, 2022

Change the values to darkGray.primary

This seems to be the correct short term solution, especially if we want to apply these changes in time for WP 6.0

Good point, I agree this is the quickest way to go if we want to land it in WP 6.0.

@jasmussen
Copy link
Contributor

This attention to detail is excellent, thank you. We definitely want to retire #50505, and #000000 is a color we should very rarely use — mostly for shadows, not for UI elements.

To an extent, the whole list of colors in this file is wrong and outdated, and exists — I believe — mainly due to it being created at a time in the development where much of the design refresh from #18667 was still underway. In that light, I would consider lines 6-15 from the sass file as the canonical and single list of grayscale colors we should support in the component set, with var(--wp-admin-theme-color) rounding out the spot color.

If I had my way, I'd rename the G2 color palette to something more sensible, it was just a temporary codename. I'd also retire all the other palettes, DARK_GRAY, DARK_OPACITY, DARK_OPACITY_LIGHT, LIGHT_GRAY, LIGHT_OPACITY_LIGHT, and BLUE. No-one should use those colors, and if we need to maintain them for backwards compatibility, we should seek out a way to clearly communicate they are deprecated.

@ciampo
Copy link
Contributor

ciampo commented Apr 7, 2022

Thank you @jasmussen , this is excellent feedback which really helps us to move in the right direction 😄

I guess next steps could be:

  • Open PR(s) to change the text color of components to darkGray.primary
  • Later, start tidying up our config according to Joen's suggestions, with the aim to consolidate the remaining colors to CSS variables.

@mirka
Copy link
Member Author

mirka commented Apr 15, 2022

I'll close this immediate issue with #40391, and move the broader cleanup to #40392.

@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants