Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Update @wordpress/base-styles and replace deprecated variables #4759

Merged
merged 36 commits into from
Jul 17, 2020

Conversation

samueljseay
Copy link
Contributor

@samueljseay samueljseay commented Jul 7, 2020

Fixes #4732

This updates @wordpress/base-styles, and because @wordpress/components depends on variables from it, that must be updated as well.

There are 3 major changes required as part of updating:

  1. The $theme-color variable is no longer exposed by base-styles. Instead there are 3 css vars exposed. These are made available by use of a provided mixin. Situations where $theme-color was darkened using scss have been mapped as best as possible to the 2 other darkened shades of the css var that are available such as --wp-admin-theme-color-darker-20 and --wp-admin-theme-color-darker-10. In some cases this means the colors are not exactly the same as before.

  2. The post css theme() call is no longer available. All uses of this have been consolidated to use of the main theme css var --wp-admin-theme-color. This means that calls like theme(secondary) or theme(outline) etc have all been consolidated to the one color.

  3. Many of the variables used for different shades of gray have been deprecated. These have been mapped across to the new gray variables. (Mapped according to the list described in Update deprecated color vars #4732)

Accessibility

Screenshots

One notable difference is the hover color of calendar days on the date picker. Due to there not being any use of secondary any more the color change is more subtle. Gif below:

cap-style

Detailed test instructions:

See the testing instructions from #4558

Theme color settings should match those in the user preferences, but now just shades of the primary color from the theme.

Additional note

I've reached out to Jay to request that he take a look at the approach taken with consolidating colors here. But other reviews are welcome!

@samueljseay samueljseay added needs: design The issue requires design input/work from a designer. tool: monorepo infrastructure focus: components Issues for woocommerce components Packages and removed tool: monorepo infrastructure focus: components Issues for woocommerce components Packages labels Jul 7, 2020
Copy link
Collaborator

@psealock psealock left a comment

Choose a reason for hiding this comment

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

This is looking great @samueljseay, thanks for the cleanup.

The code looks good, but 2 comments: One for @jameskoster and the other about removing our usage of core-* colors.

@jameskoster, this changes the button colors away from Core WP's buttons. Is that the intention for Gutenberg?

Core WordPress button on Ectoplasm theme:
Screen Shot 2020-07-08 at 2 38 11 PM

WC-Admin button after this update, also on Ectoplasm (previously it looked just like above):
Screen Shot 2020-07-08 at 2 41 04 PM

client/profile-wizard/style.scss Show resolved Hide resolved
$dark-gray-600: $core-grey-dark-600;
$dark-gray-700: $core-grey-dark-700;
$dark-gray-800: $core-grey-dark-800;
$dark-gray-900: $core-grey-dark-900;
$gray-900: $core-grey-dark-900;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that these overrides existed before this PR, but I'm having trouble remembering why they would be here. We're just taking colors handed down from Gutenberg and reassigning new colors. Shouldn't we just use $gray-900 instead of our own $core-grey-dark-900? If I'm understanding correctly, we should replace every instance of $core-grey-dark-900 with $gray-900 in an effort to remove our _colors.scss.
I think this is what is intended by #4732.

Copy link
Member

@jameskoster jameskoster Jul 9, 2020

Choose a reason for hiding this comment

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

+1. The only color var we should create is the WooCommerce purple, for the very few cases in which we need to use it.

Edit: Oh, and the colors we use for the charts in Analytics :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I wondered about that one. Most of this was just a find replace. I'll fix this.

@psealock psealock added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed [Status] Needs Review labels Jul 8, 2020
@jameskoster
Copy link
Member

@jameskoster, this changes the button colors away from Core WP's buttons. Is that the intention for Gutenberg?

The buttons should match core. FWIW they are matching for me on this branch 🤔

I noticed a few small details that are off though. I'm not sure if they're all 100% down to us, or should be addressed in this PR, but I'm going to list them here anyway :)

The purple / blue details in the screenshots above should be mapped to the theme color.

@samueljseay
Copy link
Contributor Author

samueljseay commented Jul 9, 2020

@jameskoster @psealock

How can we match other colors from the theme now that base-styles only exposes 3 shades of just one color of the theme?

(ie if we want the buttons to be green in the Ectoplasm case)

@samueljseay samueljseay self-assigned this Jul 10, 2020
@jameskoster
Copy link
Member

For the buttons in particular we should be using the core component out of the box with no overrides, so the theme color should be applied automatically.

For our custom components, maybe we can look at core components for implementation inspiration there? I assume they tackled this issue already :D

@samueljseay
Copy link
Contributor Author

samueljseay commented Jul 14, 2020

@psealock @jameskoster I have tried to be pragmatic about the refactoring of all the grays here, will need some feedback.

I found that if I tried to map everything directly to a gray that exists in the @wordpress/base-styles that it dramatically changes the look and feel of screens like analytics and the actions side menu, it makes the entire UI look quite dark and in many places worse if we try to map things as best as possible. I really struggled with this, because I couldn't actually make it look right and with no real baseline to go against I think this approach wasn't going to work.

I have opted instead to introduce one more gray variable for WooCommerce. The gray is called $gray-50 and is lighter than any grays in base-styles, but allows us to map most other grays in a better way, it is a blueish light gray shade that existed already in Woo and was used in various places. My thought is that we could look to refactor this gray away later but I think it would require a refactor of the design for those screens like Analytics that heavily rely on grays throughout.

2 relevant commits:

  1. I adjusted some direct mappings of one gray to another that felt wrong and made the UI look "wrong": ffe9c16
  2. I introduced a new gray variable "$gray-50". I have used it sparingly to recreate the look and feel of existing screens: cdff94e

Also I have mapped existing grays:

  • Replace dark-gray-primary with gray-900
  • Replace medium-gray-text with gray-700
  • Replace core-grey-dark-900 with gray-900
  • Replace core-grey-light-100 with gray-100
  • Replace core-grey-light-200 with gray-200
  • Replace core-grey-light with gray-100
  • Replace core-grey-light 400 & 500 with grey-100
  • Replace core-grey-light-600 with gray-200
  • Replace core-grey-light-900 with gray-400
  • Replace core-grey-dark 150 with gray-600, remove core-grey-dark 100,200
  • Replace core-grey-dark-300 with gray-700
  • Remove core-grey-dark-600
  • Replace core-grey-dark-400 with gray-700
  • Replace core-grey-dark-500 with gray-700.
  • Replace core-grey-dark-700 with gray-900.
  • Replace core-grey-dark-800 with gray-900.
  • Replace core-form-grey with gray-700

@samueljseay
Copy link
Contributor Author

samueljseay commented Jul 14, 2020

For the buttons in particular we should be using the core component out of the box with no overrides, so the theme color should be applied automatically.

For our custom components, maybe we can look at core components for implementation inspiration there? I assume they tackled this issue already :D

@jameskoster I DM'd you a question about this, but I'll also put it here:

Do you mean the Gutenberg button component in @wordpress/components? The reason I ask is, I had a look at the
code there and it indeed only deals with the theme css color in varying shades that is provided by base-styles.
So to me there are a couple of issues here, maybe you can clarify your thoughts on them:

  1. Gutenberg is moving towards using just a single color from the theme throughout the base look and feel.
    Older WordPress buttons don’t do this, they use multiple colors from the theme
  2. I just don’t know how we can use multiple colors from the theme if we’re just picking up the only css vars we now have available, which are three shades of one theme color? In the ectoplasm it is just the purple that is exposed.

@jameskoster
Copy link
Member

Adding a TLDR of what I said on Slack:

We should align our colors with Gutenberg. In this case that means wc-admin buttons with the Ectoplasm theme active should be purple. This is different to wp-admin where they are green, but matches Gutenberg, so that is fine.


I noticed these small issues are still outstanding;

Wherever you see the Woo purple, or blue in the screenshots above, let's use the theme color var.


Let's try to avoid adding a new custom gray variable, it will only cause headaches down the road. If you can set everything currently using your gray-50 var to gray-100 I am happy to go through with a fine-tooth comb and make adjustments :)

@jameskoster
Copy link
Member

Opened a PR on this branch here, to remove the gray-50 var and harmonise the home screen (and some analytics) colors.

@becdetat
Copy link
Contributor

Everything looks good to me apart from a couple of small things after a quick smoke test and scanning the code changes.

samueljseay and others added 23 commits July 17, 2020 11:34
These were made specifically to correct the look and feel of the
inbox and activity card UI.
* Revert "Introduce an interim gray variable till the design can be refactored."

This reverts commit cdff94e.

* Home screen color updates

* Analytics colors
@samueljseay samueljseay merged commit 7c1299c into main Jul 17, 2020
@samueljseay samueljseay deleted the update/base-styles branch July 17, 2020 00:11
samueljseay added a commit that referenced this pull request Jul 21, 2020
Fixes #4566 **(and is dependent on the changes from #4759 )**

Changes:

* Use the new `Card` and `Flex` from `@wordpress/components`
* Add contextual tooltips
* Adjust existing styles to match [new designs](https://www.figma.com/file/JH9XMFUCOjfXdr3N09AHRD/On-boarding-iterations-June-'20)
@psealock
Copy link
Collaborator

Nice job on this one team and especially @samueljseay for seeing this one through

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
focus: components Issues for woocommerce components needs: design The issue requires design input/work from a designer. tool: monorepo infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update deprecated color vars
5 participants