Skip to content

ProgressBar: Use the theme system accent for indicator color #53347

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

Merged
tyxla merged 2 commits into from
Aug 7, 2023
Merged

ProgressBar: Use the theme system accent for indicator color #53347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3aad5b5
ProgressBar: Use the theme system accent for indicator color
tyxla Aug 4, 2023
a55f9e3
Add CHANGELOG entry
tyxla Aug 4, 2023
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
- `MenuItemsChoice`, `MenuItem`: Support a `disabled` prop on a menu item ([#52737](https://github.com/WordPress/gutenberg/pull/52737)).
- `TabPanel`: Introduce a new version of `TabPanel` with updated internals and improved adherence to ARIA guidance on `tabpanel` focus behavior while maintaining the same functionality and API surface.([#52133](https://github.com/WordPress/gutenberg/pull/52133)).
- `Theme`: Expose via private APIs ([#53262](https://github.com/WordPress/gutenberg/pull/53262)).
- `ProgressBar`: Use the theme system accent for indicator color ([#53347](https://github.com/WordPress/gutenberg/pull/53347)).

### Bug Fix

Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/progress-bar/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export const Indicator = styled.div< {
top: 0;
height: 100%;
border-radius: ${ CONFIG.radiusBlockUi };
background-color: ${ COLORS.ui.theme };
Copy link
Member

@mirka mirka Aug 4, 2023

Choose a reason for hiding this comment

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

Curious, was this not working? The value is supposed to be pretty much the same:

gutenberg/packages/components/src/utils/colors-values.js

Line 34 in 904fd24

theme: 'var(--wp-components-color-accent, var(--wp-admin-theme-color, #3858e9))',

Copy link
Member Author

@tyxla tyxla Aug 7, 2023

Choose a reason for hiding this comment

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

Hmm, today I can no longer reproduce the scenario. But I'd still prefer to keep it consistent with the other color styles for this component. I realize it's very unlikely the underlying COLORS constant to change, but still, I appreciate that when looking at the styles of the progress bar component it's immediately clear what colors we're using, without having to peek at the COLORS definition. Will go ahead and merge, but let me know if you have any second thoughts about it.

Copy link
Member

@mirka mirka Aug 8, 2023

Choose a reason for hiding this comment

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

Would it be better if we renamed the COLORS.ui.theme variable to something that evokes the underlying CSS variable, like COLORS.theme.accent? For example, in Sass we have it set up like this.

In any case I don't think it makes sense to do var( --wp-components-color-accent, ${ COLORS.ui.theme } ) because that renders an entirely unnecessary level of nesting:

var( --wp-components-color-accent, var( --wp-components-color-accent, var( --wp-admin-theme-color, #3858e9 ) ) )

Copy link
Member Author

@tyxla tyxla Aug 9, 2023

Choose a reason for hiding this comment

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

You're right about the level of nesting, I'd be happy to follow up and remove it. I just want to discuss the best approach to do it.

So, I think the problem comes from the fact that in the components package, we call it accent color, but historically we've also called that color just "admin theme color".

I'd be happy with the rename as you suggested it, but would it make sense for all the pre-existing COLORS.ui.theme use cases? With that concern in mind, should we just create an alias and keep both COLORS.ui.theme and COLORS.theme.accent? Or you'd be fine with just renaming all instances to the latter?

Copy link
Member

@mirka mirka Aug 9, 2023

Choose a reason for hiding this comment

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

in the components package, we call it accent color, but historically we've also called that color just "admin theme color".

Right. It was intentional on our part to decouple it from the wp-admin theme color. And the components Theme system involves other colors like background color and grayscales as well, so we can't just call it "the theme color".

I'd be happy with the rename as you suggested it, but would it make sense for all the pre-existing COLORS.ui.theme use cases?

Yes 🎉 We haven't done the grayscale stuff yet, but the accent colors used in all our components are Theme-ready (#45289). Meaning, every use of COLORS.ui.theme and COLORS.ui.themeDark10 in our codebase is already hand-verified to look as expected in a Theme context. So it would be perfectly ok to rename them all to COLORS.theme.accent and COLORS.theme.accentDarker10 to better reflect the underlying CSS variable names.

FYI the colors-values.js file is not completely cleaned up yet. It's likely that we'll eventually remove most of the things in the COLORS.ui object, and move the rest of them to COLORS.theme.

Copy link
Member Author

@tyxla tyxla Aug 10, 2023

Choose a reason for hiding this comment

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

Sounds good, thanks for confirming @mirka! I'm planning to follow up on this next week!

Copy link
Member Author

@tyxla tyxla Aug 14, 2023

Choose a reason for hiding this comment

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

@mirka I'm renaming the color values as you suggested in #53631. Furthermore, I'm removing the duplication you outlined above in #53632. Let me know what you think!

background-color: var( --wp-components-color-accent, ${ COLORS.ui.theme } );

${ ( { isIndeterminate, value } ) =>
isIndeterminate
Expand Down