Color Schemes: Adding theming support via CSS custom properties #15731

frontdevde · 2017-06-30T16:56:20Z

This PR is the first in a series aiming to provide support for Color Schemes in Calypso.
This PR focuses on adding theming support via CSS custom properties.

Notable additions in this PR:

_theme.scss define theme variables and color schemes
_custom-properties.scss mixins for supporting CSS custom properties
dark & light color scheme for the Masterbar (to demonstrate theming using this approach)

How to test

The PR is designed to have no visual impact by default in order to make it easier to get these changes into master. By default the masterbar color scheme matches the current look 1:1. The current look also serves as the fallback for browsers not supporting CSS custom properties as well as for any hypothetically undefined variables.

The dark / light color schemes for the masterbar can be viewed by toggling their respective classes on the :root (e.g. document.documentElement.classList.toggle('is-dark-theme')).

The core ideas behind the PR and its approach are:

conveniently define a new theme in a single location
easily change predefined themes via a single class
enable instant preview of (user generated) color schemes
allow for support of user generated color schemes in the future
neatly integrates into the current CSS architecture
prevent increase in build time or overall complexity
prevent increase of overall CSS file size
keep current theme exactly as it is
treat it as a progressive enhancement (current theme remains the default)

For a more detailed description of the approach and its advantages feel free to check out my post on the subject: p4TIVU-75d-p2 (internal reference).

Screenshots of different color schemes (Masterbar):

💥 Note: This is just a first draft to give an idea of what the color schemes could look like. As the main goal for this PR is to demonstrate how the theming will work, the visual changes are intentionally limited to the Masterbar.

matticbot · 2017-07-03T11:32:20Z

@frontdevde This PR needs a rebase

frontdevde · 2017-07-11T09:57:57Z

@ehg @mtias @MichaelArestad If you have a minute it'd be awesome to get your thoughts on this one. Let me know if there's anything I can do to help move this PR along.

frontdevde · 2017-07-14T18:28:46Z

It was suggested by @jblz that it might make sense to also get some early Design Review on this one as well. I have thus added the label [Status] Needs Design Review and added @folletto to the PR. 👋

To enable a quick look at the visual changes, I've added a screenshot for each color scheme for the Masterbar to the PR description. This way it's not necessary to manually toggle classes on root.

Please note that these visual changes are just a first draft to give an idea of what the color schemes could look like. Any input/feedback is of course highly appreciated. :)

davewhitley · 2017-07-14T20:26:51Z

Wrote a big long comment and accidentally closed the browser tab, but I'll just summarize here:

👍

folletto

I left a few inline comments with clarification on the reasoning behind a few choices.

folletto · 2017-07-17T11:51:25Z

assets/stylesheets/shared/_colors.scss

+$gray-lighten-10:        lighten( $gray, 10% ); //#a8bece
+$gray-darken-10:         darken( $gray, 10% ); //#668eaa
+$gray-darken-20:         darken( $gray, 20% ); //#4f748e
+$gray-darken-30:         darken( $gray, 30% ); //#3d596d


I see these are explicit. We decided against listing all the variables a while ago, as there's no additional clarity in setting explicit variables. What's the rationale between this change?

First of all thanks a lot for your feedback, @folletto!

You're right, I'd agree that listing the variables like this doesn't necessarily add clarity. What I'd argue though is that it does help to prevent the ab(use) of color functions like lighten/darken.

Let me give an example: While working on this feature, I discovered seven different variations of a single color via color functions in a single component (4%, 5%, 10%, 12%, 13%, 17%, 100% of Wordpress Blue). This defeats the color consolidation efforts behind using color variables, compiled it's essentially seven different colors. Color functions like lighten/darken make it very easy to use and create colors that are off-brand.  

Thus in order to discourage the use of color functions, my idea here was the following: we could explicitly declare all colors in the WordPress.com design handbook in the colors file and encourage the explicit use of the color variables as defined in this file only. These should cover most cases of blues and grays. If further darker/lighter shades of other colors are needed those too should be added to the colors file and then used explicitly. This way we can easily keep track of all (shades of) colors used in one place and ensure that we're staying on-brand.

This was the rationale behind adding these explicit color variables. Would be great to get your thoughts on this. Does this sound like a feasible way to reduce color variations to you?

reduce color variations

It's not a problem we have, or that we intended to "fix" however. We purposefully chose this approach exactly for its flexibility, yet still grounded in the variables.

It's a discussion we could have, but as far as I'm aware it didn't emerge so far as a problem, as such I don't think we should try to fix it. Maybe it's just because the discussion didn't happen yet, but... I wouldn't use the theming PR to change one of the fundamentals of our color approach.

We didn't want the % values to be too limiting, so we didn't make them variables. It was more intended for $gray, because we needed to enable designers to pick the shade of gray they needed. We probably shouldn't have so many different %s of WordPress Blue. I guess the lesson is that even though we don't explicitly say you can use darken/lighten for WordPress Blue, ppl will do it anyway :)

That being said, I'm not totally against using variables for them if it's necessary. It doesn't prohibit us from using a custom % when we want to. I agree, maybe a separate PR would be better.

folletto · 2017-07-17T11:52:38Z

assets/stylesheets/shared/_colors.scss

+$red-error:              #d94f4f;
+$alert-yellow:           $yellow-warning;
+$alert-red:              $red-error;
+$alert-green:            $green-valid;


Similarly to the above, what's the rationale behind the extra complexity of this indirection of the variables?

Based on the rationale explained in my comment above, I added all colors from the WordPress.com design handbook to the colors file. In order to make their use as consistent and straightforward as possible, I named them the way they are called in the design handbook. In the case of these three variables, alternative variables already existed that didn't match the design handbook names. But as they are in use in a lot of instances throughout our code base, I opted for initially just reassigning the variables in this PR. Giving the high number of occurrences, I assumed changing these variables would be cleaner in its own PR.

The colors however are called $alert-red in the Handbook not $red-error – and it's currently consistently used on web, iOS and Android. From the Handbook:

Let's not introduce new variables and new indirections, nor use a PR to do two changes in one — PRs should be as much as sensibly relevant doing just one thing, otherwise it's hard to discuss, review, and find the decisions later.

folletto · 2017-07-17T11:53:53Z

assets/stylesheets/shared/_themes.scss

+$masterbar-border-color:           darken( $masterbar-bg-color, 4% );
+$masterbar-item-active-bg-color:   darken( $masterbar-bg-color, 17% );
+$masterbar-item-hover-bg-color:    lighten( $masterbar-bg-color, 5% );
+$masterbar-item-new-color:         $blue-wordpress;


Ideally the color shouldn't be assigned per-component like this, but just override the individual variables. Is this needed just in the scope of this PR targeting just the masterbar, and will be removed later?

folletto · 2017-07-17T11:57:21Z

client/layout/masterbar/style.scss

+			'border-color': --masterbar-border-color,
+			'color': --masterbar-color,
+		)
+	);


This seems adding opacity to the simple assignment of a variable. Do we really need a special syntax instead of simply three lines? What's the benefit?

background: var( --masterbar-bg-color ); border-color: var( --masterbar-border-color ); color: var( --masterbar-color );

I agree with this. I also would recommend a more descriptive mixin name.

@folletto I've added a comment to your other question below as to why we'd want to use a mixin when using custom properties (tl;dr: to provide a fallback).

The idea behind this mixin in particular though was to improve the developer experience by providing a mixin that allows the developer to declare multiple properties at once. The same could, of course, be achieved by using the theme mixin three times:

@include theme( background, --masterbar-bg-color ); @include theme( border-color, --masterbar-border-color ); @include theme( color, --masterbar-color );

It does potentially reduce legibility of the code though and thus the potential long term cost might outweight the short term developer experience benefit. Maybe just remove it?

Yep, I think it's better to use the latter, as it would avoid learning and understanding two syntaxes that do the same, but also importantly would keep the indentation in the SCSS files uniquely for the class hierarchy.

As for the name, maybe: theme-color or just color?

UPDATE: but see the other comment below as this might not apply anymore.

folletto · 2017-07-17T11:59:19Z

client/layout/masterbar/style.scss

@@ -38,7 +43,7 @@ $autobar-height: 20px;
 	display: flex;
 		align-items: center;
 	position: relative;
-	color: $white;
+	@include theme( color, --masterbar-color );


Why do we need an include? Can't we just use var assignment? We should strive to have one and only one way to define colors unless there are clear technical needs (in which case we need to assess if it's really beneficial).

color: var( --masterbar-color );

The include does a bit more behind the scenes. In addition to adding the themeable variable, the include outputs a color fallback. As outlined in a bit more detail in p4TIVU-75d-p2 (internal reference) we need to provide a color fallback (the default color scheme) for browsers that do not support CSS custom properties yet.

Therefore @include theme( color, --masterbar-color ); compiles to

color: $masterbar-color; color: var( --masterbar-color );

This way we ensure that there is always a fallback provided for each custom property.

Sorry yes, I was confused that when we decided to keep IE11 as being progressive enhancement (thus, no themes) we solved this issue. As you said, it's not the case.

...

I gave a thought about it, and I narrowed down why this feels odd to be done in this way: it's the same scope for which autoprefixer exists, right? The scenario is that we don't want to care about writing any different SCSS/CSS for backward compatibility, we let the processor to deal with it.

So, I'd expect that the above syntax to be just:

color: var( --some-theme-color, $wordpress_blue );

And that this would be expanded by PostCSS or similar processor to:

color: #0087be; color: var( --some-theme-color, #0087be );

...probably through an intermediate step that replaced the SCSS variable.

Can you look into this?

💯 Love it! Abstracting the additional complexity away and doing the magic behind the scenes would not only increase the developer experience but more importantly enable us to simply pull the plug on the post-processing step once the fallback isn't necessary anymore. Great input, I'll look into it. 👍

folletto · 2017-07-17T12:00:10Z

The PR is designed to have no visual impact by default in order to make it easier to get these changes into master.

+1 good approach

_theme.scss

Any benefit having just one file instead of one file for each theme?

{ code review }

I added a few question in the code review. :)

MichaelArestad · 2017-07-18T16:43:13Z

Any benefit having just one file instead of one file for each theme?

While I don't think any colors outside of our brand colors should be in our color variables file, I do think the one file makes some sense. I find it pretty straightforward to find and manage the color schemes since they are just relatively simple objects in a Sass map. It could be broken up, but I don't know we get a major benefit either way. If we do break it up, it would be more akin to our component mindset.

MichaelArestad · 2017-07-18T16:44:49Z

client/components/sub-masterbar-nav/style.scss

@@ -3,7 +3,7 @@
 	display: flex;
 	flex-direction: column;
 	padding: 56px 20px 28px;
-	background-color: $masterbar-color;
+	background-color: $masterbar-bg-color;


Can you use full words in the variables rather than shortened abbreviations?

Good point! While I used bg here in order to stay consistent with the already existing $sidebar-bg-color, I do agree that in both cases we probably would want to use the full words.

MichaelArestad · 2017-07-18T16:46:21Z

Other than a few minor comments and questions, I think this is looking fairly nice. Good work so far!

frontdevde · 2017-07-18T23:06:01Z

Any benefit having just one file instead of one file for each theme?

Having one file for the color schemes removes complexity from working with and extending themes. Personally, I like the simplicity of adding/extending/removing themes using a single map in one file, rather than having to edit multiple files. Especially considering our limited number of color schemes. Definitely open to discussing a multi-file approach though if there are any important benefits that we might miss out on otherwise.

frontdevde · 2017-07-18T23:12:36Z

@folletto @MichaelArestad
Thanks so much for chiming in here, your input is highly appreciated! I just added some comments on here and have a few changes coming up based on your feedback. 👍

frontdevde · 2017-08-14T04:50:55Z

Quick update: I've been working on breaking this PR up in smaller portions. Thanks again for your feedback, the new PRs are going to address the suggestions mentioned here. I'll close this one out once the new PRs are up.

frontdevde · 2017-08-19T15:56:13Z

Thanks for your input! Closing this PR in favour of 👉 PR #17366

frontdevde added OSS Citizen [Status] In Progress labels Jun 30, 2017

frontdevde self-assigned this Jun 30, 2017

frontdevde requested review from ehg, mtias and MichaelArestad June 30, 2017 16:56

matticbot added OSS Citizen [Size] XL Probably needs to be broken down into multiple smaller issues labels Jun 30, 2017

frontdevde added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jun 30, 2017

matticbot added the [Status] Needs Rebase label Jul 3, 2017

frontdevde added 9 commits July 11, 2017 12:34

Update color variables to match design handbook

f9dff15

Make map-deep-get its own function for reuse

bf23a14

Create themes file and move layout variables there

4753cf5

Add themes description and update comments

aa7d2ab

Add color schemes map

0703f5e

Add mixins for usage of custom properties with fallback properties

c7045a7

Add theming classes and inline documentation

a82c782

Add mixin for theming multiple properties at once

990c4bb

Add theming to the masterbar

fadb4c1

frontdevde force-pushed the add/color-schemes/style-groundwork branch from 1458f24 to fadb4c1 Compare July 11, 2017 09:40

frontdevde added [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] Needs Rebase labels Jul 14, 2017

frontdevde requested a review from folletto July 14, 2017 18:17

folletto suggested changes Jul 17, 2017

View reviewed changes

MichaelArestad reviewed Jul 18, 2017

View reviewed changes

folletto mentioned this pull request Jul 28, 2017

Accessibility: Provide high contrast interface option #457

Closed

karmatosed mentioned this pull request Jul 28, 2017

Accessibility: visual enhancements #16699

Closed

4 tasks

frontdevde closed this Aug 19, 2017

matticbot removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 19, 2017

frontdevde mentioned this pull request Aug 19, 2017

Color Schemes: Add color schemes via CSS custom properties #17366

Closed

alisterscott deleted the add/color-schemes/style-groundwork branch October 24, 2017 04:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Color Schemes: Adding theming support via CSS custom properties #15731

Color Schemes: Adding theming support via CSS custom properties #15731

frontdevde commented Jun 30, 2017 •

edited

Loading

matticbot commented Jul 3, 2017

frontdevde commented Jul 11, 2017

frontdevde commented Jul 14, 2017 •

edited

Loading

davewhitley commented Jul 14, 2017 •

edited

Loading

folletto left a comment

folletto Jul 17, 2017

frontdevde Jul 18, 2017

folletto Jul 19, 2017

davewhitley Jul 19, 2017 •

edited

Loading

folletto Jul 17, 2017

frontdevde Jul 18, 2017

folletto Jul 19, 2017

folletto Jul 17, 2017

folletto Jul 17, 2017

MichaelArestad Jul 18, 2017

frontdevde Jul 18, 2017

folletto Jul 19, 2017 •

edited

Loading

folletto Jul 17, 2017

frontdevde Jul 18, 2017

folletto Jul 19, 2017

frontdevde Jul 19, 2017

folletto commented Jul 17, 2017

MichaelArestad commented Jul 18, 2017

MichaelArestad Jul 18, 2017

frontdevde Jul 18, 2017 •

edited

Loading

MichaelArestad commented Jul 18, 2017

frontdevde commented Jul 18, 2017 •

edited

Loading

frontdevde commented Jul 18, 2017 •

edited

Loading

frontdevde commented Aug 14, 2017

frontdevde commented Aug 19, 2017

Color Schemes: Adding theming support via CSS custom properties #15731

Color Schemes: Adding theming support via CSS custom properties #15731

Conversation

frontdevde commented Jun 30, 2017 • edited Loading

Notable additions in this PR:

How to test

The core ideas behind the PR and its approach are:

Screenshots of different color schemes (Masterbar):

matticbot commented Jul 3, 2017

frontdevde commented Jul 11, 2017

frontdevde commented Jul 14, 2017 • edited Loading

davewhitley commented Jul 14, 2017 • edited Loading

folletto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davewhitley Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

folletto Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

folletto commented Jul 17, 2017

MichaelArestad commented Jul 18, 2017

Choose a reason for hiding this comment

frontdevde Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

MichaelArestad commented Jul 18, 2017

frontdevde commented Jul 18, 2017 • edited Loading

frontdevde commented Jul 18, 2017 • edited Loading

frontdevde commented Aug 14, 2017

frontdevde commented Aug 19, 2017

frontdevde commented Jun 30, 2017 •

edited

Loading

frontdevde commented Jul 14, 2017 •

edited

Loading

davewhitley commented Jul 14, 2017 •

edited

Loading

davewhitley Jul 19, 2017 •

edited

Loading

folletto Jul 19, 2017 •

edited

Loading

frontdevde Jul 18, 2017 •

edited

Loading

frontdevde commented Jul 18, 2017 •

edited

Loading

frontdevde commented Jul 18, 2017 •

edited

Loading