Build: using a postcss plugin to generate the admin-schemes styles by youknowriad · Pull Request #6739 · WordPress/gutenberg

youknowriad · 2018-05-14T13:41:52Z

This PR adds a PostCSS plugin to generate the admin-schemes automatically at build-time.
This has some pros over the previous approach.

We don't need to write the specific rules ourselves, they are automatically extracted from the styles
The admin schemes styles are generated per stylesheet which means they are not generated in the edit-post style. If you load just the components stylesheet, it will come bundled with the admin-schemes styles.

Usage:

Instead of using colors directly in our rules, we need to use these helpers:

color: theme(primary)

The argument passed to the theme helper is one of the defined theme colors (primary/secondary/primary-dark15 for now, we can add as much as colors as we want)

Some drawbacks:

We can't use darken/lighten on theme colors, because the Sass loader is run before the post-css transformation, for example I had to define a primary-dark15 theme color to replace the previous call to darken( theme(primary), 15 )
The date picker styles were overriden using sass variables and since the date picker styles uses darken/lighten, we can't do this anymore, we'd have to figure out another way to override those.

This PR will help with #6562

Testing instructions

Switch your user's profile and check that the theme colors are updating properly.

aaronjorbin · 2018-05-14T14:42:32Z

For lighten/darken, you can use https://github.com/postcss/postcss-color-function you can use the CSS colors modules which are still in draft https://drafts.csswg.org/css-color/#modifying-colors

ntwb · 2018-05-15T01:00:05Z

+							},
+						},
+					} ),
 					require( 'autoprefixer' ),


Should autoprefixer be installed as a dependency for this?
And @wordpress/browserslist-config for the Autoprefixer configuration.

I don't think autoprefixer should be a dependency, it's a separate postCSS plugin. I'll add the browserlist config.

Ah, right you are, my bad, this is Gutenberg's webpack.config.js file ;)

ntwb · 2018-05-15T01:01:19Z

+  "postcss": "^6.0.16",
+  "postcss-value-parser": "^3.3.0"
+ }
+}


Can this be reformatted to use tabs for indentation please

ntwb · 2018-05-15T01:03:00Z

Awesome, I ❤️ PostCSS

p.s. http://postcss.parts is a great resource when searching for PostCSS plugins

jasmussen · 2018-05-15T06:45:23Z

I love that there's so much hard-coded CSS that's suddenly red in this PR. Nice work. I also like that you were able to port the primary and secondary colors both. I look forward to one day when we can style the whole admin using postCSS like this 🤞

The switch isn't styled, though, and this might be the tricky aspect, that will need a little TLC.

As part of #6438, I made it so the switch is never "warning colored". You can see how that's done here: https://github.com/WordPress/gutenberg/blob/master/edit-post/assets/stylesheets/main.scss#L8

@include admin-scheme-color-overrides( 'midnight', $scheme-midnight__spot-color, $scheme-midnight__spot-color-2 ); // exception to ensure usability
@include admin-scheme-color-overrides( 'ocean', $scheme-ocean__spot-color, $scheme-ocean__spot-color );
@include admin-scheme-color-overrides( 'sunrise', $scheme-sunrise__spot-color, $scheme-sunrise__spot-color-2 ); // exception to ensure usability

In nearly all rules, the switch is the same color as the Publish button. But in Sunrise and Midnight, the switch uses the 2nd spot color, to avoid using warning colors such as red and orange in this case. We need to ensure that's still the case here.

youknowriad · 2018-05-15T07:30:26Z

In nearly all rules, the switch is the same color as the Publish button. But in Sunrise and Midnight, the switch uses the 2nd spot color, to avoid using warning colors such as red and orange in this case. We need to ensure that's still the case here.

The good thing about this PR is that it's easy to just introduce a new named color specific to the switch and controls that need to behave similarly to solve this issue.

… as theme colors

ntwb · 2018-05-15T08:19:19Z

+	"main": "src/index.js",
+	"dependencies": {
+		"postcss": "^6.0.16"
+	}


Can you add the Lerna config after this line please:

, "publishConfig": { "access": "public" }

youknowriad · 2018-05-15T08:21:49Z

@jasmussen I updated the PR and tried to fix the colors of the switches. Let me know if it's correct.

The reason, they were not being transformed is that they were not using the regular "primary" color in the default theme but a slightly different shade.

jasmussen · 2018-05-15T08:44:33Z

@jasmussen I updated the PR and tried to fix the colors of the switches. Let me know if it's correct.

Cool. It's now colored. However it's the wrong color, and this is the problem I was referring to before.

Every color scheme but Midnight and Sunrise should use the primary color for the toggle control.

Midnight and Sunrise should use the secondary color for the toggle control.

Wrong color for Midnight:

Right color for Midnight:

youknowriad · 2018-05-15T08:58:47Z

@jasmussen I added a "toggle" color to fix this. Let me know what you think?

jasmussen · 2018-05-15T09:04:13Z

Nice! I can confirm that all colors look right now on the switch. Well done.

gziolo · 2018-05-15T09:13:05Z

@@ -0,0 +1,55 @@
+const postcss = require( 'postcss' );


it doesn't need to be transpiled, should we put in the top folder?

I personally prefer to be consistent. Maybe we should introduce an exclusion rule in our potential build script instead

I think we should raise this in the #core-js chat, I'm leaning towards consistency and thinking we should put all @wordpress packages into /src folders.

With more and more packages being released and the increase in contributors looking to grok all of @WordPress' packages, consistency in package layout and formatting I think would help new users and contributors when first exploring all the new shiny things WordPress are putting forward.

''Not a blocker, so can be decided at a later date''

''Not a blocker, so can be decided at a later date''

Agreed 👍

gziolo · 2018-05-15T09:20:20Z

@@ -0,0 +1,13 @@
+# @wordpress/postcss-themes


For consistency, can we name it postcss-themes-webpack-plugin?
See https://github.com/WordPress/packages/tree/master/packages/custom-templated-path-webpack-plugin.

Well this is not a webpack plugin, it's a postcss plugin and the name is following the postcss rules.

Agreed, it is a PostCSS plugin.

I will note that I preemptively went to check on the name at PostCSS.parts

https://www.postcss.parts/?searchTerm=theme

A PostCSS plugin already exists with the name postcss-theme https://github.com/andywer/postcss-theme, this package here in this PR is scoped with @wordpress so our package name would be @wordpress/postcss-theme

I don't see any issues personally with the @wordpress/postcss-theme name but just wanting to make a note of it regardless for if others have any issues with the name.

Sorry, I didn't quite get it :D

Skip it.

gziolo · 2018-05-15T09:23:41Z

 		color: $white !important;
 		background-size: 100px 100% !important;
-		background-image: linear-gradient( -45deg, $blue-medium-500 28%, $blue-dark-900 28%, $blue-dark-900 72%, $blue-medium-500 72%) !important;
+		background-image: linear-gradient( -45deg, theme(primary) 28%, $blue-dark-900 28%, $blue-dark-900 72%, theme(primary) 72%) !important;


Don't want to add you too much work in case there is no linter, but noting that we use spacing in scss files around parenthesis. 🤷‍♂️

Yes, I'll update but we should definitely add a linter ;)

gziolo · 2018-05-15T10:53:59Z

One of the tests fails because of the console.warning call. We should either remove the warning or include it in the test expectations.

Otherwise, everything looks great. Thanks for adding tests. They read great and snapshots are very helpful to understand how it is supposed to work.

jasmussen · 2018-05-16T07:04:43Z

I just now noticed that $blue-medium-500 has been removed. I am guessing because this is now a "theme color". But I'm not sure we should remove it, as the range from 100-900 now misses one in the middle.

Basically I feel like the medium blues should exist as the base, and themes can then add on top of that. In other words, I think we should restore $blue-medium-500 as a distinct variable, even if the theme color simply references it. What do you think?

gziolo · 2018-05-16T07:20:51Z

@jasmussen sounds, like a good idea given it might bring some confusion, unless @youknowriad knows good reasons to avoid it :)

youknowriad · 2018-05-16T08:00:44Z

Basically I feel like the medium blues should exist as the base, and themes can then add on top of that. In other words, I think we should restore $blue-medium-500 as a distinct variable, even if the theme color simply references it. What do you think?

I think I prefer to remove all the colors instead :). If the colors are available we'll be tempted to use them but this will not translate to other themes while if we remove them we kind of force people to think about theme colors when adding UI.

Though, there's probably a middle ground where some colors are shared across all themes (maybe link colors) and in that case, it's fine to keep.

jasmussen · 2018-05-16T08:04:27Z

I think I prefer to remove all the colors instead :). If the colors are available we'll be tempted to use them but this will not translate to other themes while if we remove them we kind of force people to think about theme colors when adding UI.

I agree, long term. But near term, the color schemes are simply not advanced enough that they should dictate how the rest of the UI is designed. For now they are limited to spot colors and secondary colors that override the main theme. It would be nice long term if every theme had a full range of colors. But so long as they are in the override territory, we need to keep the range of blues that are designed for the default theme.

aduth · 2018-05-23T17:23:14Z

+
+// Extending colors to use theme colors
+.react-datepicker__time-container {
+  .react-datepicker__time {


Shouldn't these be tabs?

Try using a post-css plugin to generate the admin-schemes styles

cb86eef

youknowriad added [Type] Build Tooling Issues or PRs related to build tooling Needs Design Feedback Needs general design feedback. labels May 14, 2018

youknowriad requested review from a team and jasmussen May 14, 2018 13:41

youknowriad changed the title ~~Build: using a post-css plugin to generate the admin-schemes styles~~ Build: using a postcss plugin to generate the admin-schemes styles May 14, 2018

ntwb requested changes May 15, 2018

View reviewed changes

Add browserlist config

0eccdde

youknowriad force-pushed the try/post-css-admin-themes branch from c6b496b to 0eccdde Compare May 15, 2018 07:44

youknowriad added 2 commits May 15, 2018 08:58

Use postCSS color-function to avoid having to declare darkened colors…

3804b8d

… as theme colors

Fix postcss-themes packages.json indentation

cadccfd

ntwb reviewed May 15, 2018

View reviewed changes

Fix the form toggle colors

60e5bef

Add publishConfig to the package.json of postcss-themes

cc697b3

ntwb approved these changes May 15, 2018

View reviewed changes

youknowriad added 2 commits May 15, 2018 09:57

Fix date picker theme colors

d2dd7d9

Fix toggle colors

192fd71

youknowriad force-pushed the try/post-css-admin-themes branch from 6019023 to 192fd71 Compare May 15, 2018 08:58

gziolo reviewed May 15, 2018

View reviewed changes

Adding a unit test

bb51d0e

Fix sass code style (adding spaces)

9f63db0

Fix unit tests

2cd98fa

youknowriad merged commit f920f2d into master May 15, 2018

youknowriad deleted the try/post-css-admin-themes branch May 15, 2018 11:55

jasmussen mentioned this pull request May 16, 2018

Polish the Table block #6314

Merged

aduth reviewed May 23, 2018

View reviewed changes

youknowriad mentioned this pull request May 24, 2018

Fix/tabs indent react datepicker #6934

Merged

youknowriad mentioned this pull request Jun 5, 2018

Theme the hover outlines #7111

Merged

This was referenced Oct 1, 2018

Gutenberg: edit-post section uses theme(primary) Automattic/wp-calypso#27526

Closed

Gutenberg: replace post-css usages of theme(primary) and theme(toggle) to use static color values Automattic/wp-calypso#27625

Merged

swissspidy mentioned this pull request May 7, 2019

Admin Color Scheme Support ampproject/amp-wp#2265

Closed

Conversation

youknowriad commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronjorbin commented May 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntwb May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntwb commented May 15, 2018

Uh oh!

jasmussen commented May 15, 2018

Uh oh!

youknowriad commented May 15, 2018

Uh oh!

ntwb May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented May 15, 2018

Uh oh!

jasmussen commented May 15, 2018

Uh oh!

youknowriad commented May 15, 2018

Uh oh!

jasmussen commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo May 15, 2018 • edited by ntwb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo commented May 15, 2018

Uh oh!

jasmussen commented May 16, 2018

Uh oh!

gziolo commented May 16, 2018

Uh oh!

youknowriad commented May 16, 2018

Uh oh!

jasmussen commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

youknowriad commented May 14, 2018 •

edited

Loading

ntwb May 15, 2018 •

edited

Loading

ntwb May 15, 2018 •

edited

Loading

gziolo May 15, 2018 •

edited by ntwb

Loading