Build: using a postcss plugin to generate the admin-schemes styles #6739

youknowriad · May 14, 2018

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 · May 14, 2018

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 · May 15, 2018

ntwb requested changes May 15, 2018

View changes

ntwb · May 15, 2018

webpack.config.js

+								'primary-dark15': '#bf7030',
+							},
+						},
+					} ),
 					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 · May 15, 2018

packages/postcss-themes/package.json

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


Can this be reformatted to use tabs for indentation please

ntwb · May 15, 2018

Awesome, I ❤️ PostCSS

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

jasmussen · May 15, 2018

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 · May 15, 2018

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.

ntwb · May 15, 2018

ntwb reviewed May 15, 2018

View changes

ntwb · May 15, 2018

packages/postcss-themes/package.json

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


Can you add the Lerna config after this line please:

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

youknowriad · May 15, 2018

@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.

ntwb · May 15, 2018

ntwb approved these changes May 15, 2018

View changes

jasmussen · May 15, 2018

@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 · May 15, 2018

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

jasmussen · May 15, 2018

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

gziolo · May 15, 2018

gziolo reviewed May 15, 2018

View changes

gziolo · May 15, 2018

packages/postcss-themes/src/index.js

@@ -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 · May 15, 2018

gziolo reviewed May 15, 2018

View changes

gziolo · May 15, 2018

packages/postcss-themes/README.md

@@ -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 · May 15, 2018

gziolo reviewed May 15, 2018

View changes

gziolo · May 15, 2018

components/button/style.scss

@@ -34,7 +34,7 @@
 	&.button-primary.is-busy[disabled] {
 		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 · May 15, 2018

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 · May 16, 2018

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 · May 16, 2018

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

youknowriad · May 16, 2018

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 · May 16, 2018

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 · May 23, 2018

aduth reviewed May 23, 2018

View changes

aduth · May 23, 2018

components/date-time/style.scss

+
+// 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

Loading status checks…

cb86eef

youknowriad added [Type] Build Tooling Needs Design Feedback labels May 14, 2018

youknowriad requested review from jasmussen and WordPress/gutenberg-core May 14, 2018

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

Add browserlist config

Loading status checks…

0eccdde

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

youknowriad added some commits May 15, 2018

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

Loading status checks…

3804b8d

Fix postcss-themes packages.json indentation

Loading status checks…

cadccfd

Fix the form toggle colors

Loading status checks…

60e5bef

Add publishConfig to the package.json of postcss-themes

Loading status checks…

cc697b3

youknowriad added some commits May 15, 2018

Fix date picker theme colors

d2dd7d9

Fix toggle colors

Loading status checks…

192fd71

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

Adding a unit test

Loading status checks…

bb51d0e

Fix sass code style (adding spaces)

Loading status checks…

9f63db0

Fix unit tests

Loading status checks…

2cd98fa

youknowriad merged commit f920f2d into master May 15, 2018
2 checks passed

2 checks passed

codecov/project 46.25% (+0.19%) compared to ca2b3d3
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

jasmussen referenced this pull request May 16, 2018
Merged
Polish the Table block #6314

youknowriad referenced this pull request May 24, 2018
Merged
Fix/tabs indent react datepicker #6934

youknowriad referenced this pull request Jun 5, 2018
Merged
Theme the hover outlines #7111

This was referenced Oct 1, 2018

Closed

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

Merged

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

Feb	MAR	Apr
	30
2018	2019	2020

WordPress/gutenberg

Join GitHub today

Build: using a postcss plugin to generate the admin-schemes styles #6739

Conversation

youknowriad commented May 14, 2018 • edited

youknowriad added [Type] Build Tooling Needs Design Feedback labels May 14, 2018

youknowriad requested review from jasmussen and WordPress/gutenberg-core May 14, 2018

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

This comment has been minimized.

aaronjorbin commented May 14, 2018

ntwb requested changes May 15, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ntwb May 15, 2018 • edited

This comment has been minimized.

This comment has been minimized.

ntwb commented May 15, 2018

This comment has been minimized.

jasmussen commented May 15, 2018

This comment has been minimized.

youknowriad commented May 15, 2018

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

youknowriad added some commits May 15, 2018

ntwb reviewed May 15, 2018 View changes

This comment has been minimized.

ntwb May 15, 2018 • edited

This comment has been minimized.

youknowriad commented May 15, 2018

ntwb approved these changes May 15, 2018 View changes

This comment has been minimized.

jasmussen commented May 15, 2018

youknowriad added some commits May 15, 2018

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

This comment has been minimized.

youknowriad commented May 15, 2018

This comment has been minimized.

jasmussen commented May 15, 2018

gziolo reviewed May 15, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gziolo reviewed May 15, 2018 View changes

This comment has been minimized.

gziolo May 15, 2018 • edited by ntwb

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gziolo reviewed May 15, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gziolo commented May 15, 2018

Hide details View details youknowriad merged commit f920f2d into master May 15, 2018 2 checks passed

2 checks passed

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

This comment has been minimized.

jasmussen commented May 16, 2018

jasmussen referenced this pull request May 16, 2018

This comment has been minimized.

gziolo commented May 16, 2018

This comment has been minimized.

youknowriad commented May 16, 2018

This comment has been minimized.

jasmussen commented May 16, 2018

aduth reviewed May 23, 2018 View changes

This comment has been minimized.

youknowriad referenced this pull request May 24, 2018

youknowriad referenced this pull request Jun 5, 2018

This was referenced Oct 1, 2018

youknowriad commented May 14, 2018 •

edited

ntwb requested changes May 15, 2018

View changes

ntwb May 15, 2018 •

edited

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

ntwb reviewed May 15, 2018

View changes

ntwb May 15, 2018 •

edited

ntwb approved these changes May 15, 2018

View changes

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

gziolo reviewed May 15, 2018

View changes

gziolo reviewed May 15, 2018

View changes

gziolo May 15, 2018 •

edited by ntwb

gziolo reviewed May 15, 2018

View changes

youknowriad merged commit `f920f2d` into master May 15, 2018
2 checks passed

aduth reviewed May 23, 2018

View changes