Components: Clean up unused and duplicate COLORS values
#43445
Merged
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
mirka
added
[Type] Code Quality
|
Size Change: -704 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
mirka
commented
Aug 19, 2022
Comment on lines
+48
to
+49
| theme: 'var( --wp-admin-theme-color, #007cba)', | ||
| themeDark10: 'var( --wp-admin-theme-color-darker-10, #006ba1)', |
There was a problem hiding this comment.
These fallback values will not matter in reality because the built style.css file for the components package will include the CSS variable definitions.
ciampo
reviewed
Aug 22, 2022
| secondary: '#ccc', | ||
| tertiary: '#e7e8e9', | ||
| }, | ||
| // Matches the grays in @wordpress/base-styles |
There was a problem hiding this comment.
It would great if we could, in the future, use the same values from @wordpress/base-styles without having to manually re-state them.
We could either:
- declare those values in
@wordpress/base-stylesas CSS variables, OR - in the context of allowing some theming to
@wordpress/components, expose some CSS variables and therefore allow consumers of the components package to override these values.
The second option would be my favourite, as it decouple the package better. But I also recognize that we can't expose every single value as a CSS variable, as it can quickly grow to become overwhelming.
|
|
||
| const G2 = { |
There was a problem hiding this comment.
Good that we're getting rid of "g2" references, as they don't make much sense anymore!
| @@ -48,7 +48,6 @@ const wrapperMargin = ( { marks, __nextHasNoMarginBottom }: WrapperProps ) => { | |||
| }; | |||
|
|
|||
| export const Wrapper = styled.div< WrapperProps >` | |||
| color: ${ COLORS.blue.medium.focus }; | |||
There was a problem hiding this comment.
This line wasn't needed because color was being assigned again a few lines later (${ wrapperColor }; )
There was a problem hiding this comment.
I wrote this down in the commit message so I don't forget, and then forgot to write it as a comment 😂 Sorry to make you sleuth!
ciampo
approved these changes
Aug 22, 2022
It's only used in the RangeControl Wrapper, and it always gets overriden by another color.
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.
Part of #40392
What?
Clean up unused and duplicate color values in the
colors-values.jsfile.We also add some better JSDoc guidance to prevent usage of deprecated colors.
Why?
The color value file is overwhelming and does not match the current direction of the components package. Our ultimate goal is to simplify and adhere to the color scheme defined in
@wordpress/base-styles.How?
This first pass of the cleanup task focuses on removing unused and duplicate colors.
Testing Instructions
For easier code review, I recommend reading commit by commit.
In Storybook, do a smoke test of some of the affected components. Actual colors should not have changed.