Skip to content
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

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f9dff15
Update color variables to match design handbook
frontdevde Jun 28, 2017
bf23a14
Make map-deep-get its own function for reuse
frontdevde Jun 28, 2017
4753cf5
Create themes file and move layout variables there
frontdevde Jun 29, 2017
aa7d2ab
Add themes description and update comments
frontdevde Jun 29, 2017
0703f5e
Add color schemes map
frontdevde Jun 29, 2017
c7045a7
Add mixins for usage of custom properties with fallback properties
frontdevde Jun 30, 2017
a82c782
Add theming classes and inline documentation
frontdevde Jun 30, 2017
990c4bb
Add mixin for theming multiple properties at once
frontdevde Jun 30, 2017
fadb4c1
Add theming to the masterbar
frontdevde Jun 30, 2017
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
97 changes: 50 additions & 47 deletions assets/stylesheets/shared/_colors.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,74 +1,77 @@
/**
* Colors
*
* Color variables based on the design handbook
* See wordpress.com/design-handbook/colors/ for more info.
*/

// Primary Accent (Blues)
$blue-wordpress: #0087be;
$blue-light: #78dcfa;
$blue-medium: #00aadc;
$blue-dark: #005082;

// Jetpack Green
$green-jetpack: #8cc258;

// Grays
$gray: #87a6bc;
$gray-light: lighten( $gray, 33% ); //#f3f6f8
$gray-lighten-30: lighten( $gray, 30% ); //#e9eff3
$gray-lighten-20: lighten( $gray, 20% ); //#c8d7e1
$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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@davewhitley davewhitley Jul 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

$gray-dark: darken( $gray, 38% ); //#2e4453

// $gray-text: ideal for standard, non placeholder text
// $gray-text-min: minimum contrast needed for WCAG 2.0 AA on white background
$gray-text: $gray-dark;
$gray-text-min: darken( $gray, 18% ); //#537994

// $gray color functions:
// lighten( $gray, 10% ) //#a8bece
// lighten( $gray, 20% ) //#c8d7e1
// lighten( $gray, 30% ) //#e9eff3
// darken( $gray, 10% ) //#668eaa
// darken( $gray, 20% ) //#4f748e
// darken( $gray, 30% ) //#3d596d
//
// See wordpress.com/design-handbook/colors/ for more info.

// Secondary Accent (Oranges)
$orange-jazzy: #f0821e;
$orange-fire: #d54e21;
$orange-jazzy: #f0821e;

// Alerts
$alert-yellow: #f0b849;
$alert-red: #d94f4f;
$alert-green: #4ab866;
$alert-purple: #855DA6;

// Link hovers
$link-highlight: tint($blue-medium, 20%);
// Validations and Alerts
$green-valid: #4ab866;
$yellow-warning: #f0b849;
$red-error: #d94f4f;
$alert-yellow: $yellow-warning;
$alert-red: $red-error;
$alert-green: $green-valid;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

screen shot 2017-07-19 at 10 41 58

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.


// Essentials
$white: rgba(255,255,255,1);
$transparent: rgba(255,255,255,0);

// Uncommon
$border-ultra-light-gray: #e8f0f5;
//Additional colors

// Alerts
$alert-purple: #855DA6;

// Layout
$masterbar-color: $blue-wordpress;
$sidebar-bg-color: lighten( $gray, 30% );
$sidebar-text-color: $gray-dark;
$sidebar-selected-color: $gray-text-min;
// Jetpack
$green-jetpack: #8cc258;

// Social media colors
$color-facebook: #39579a;
$color-twitter: #55ACEE;
$color-gplus: #df4a32;
$color-tumblr: #35465c;
$color-linkedin: #0976b4;
$color-path: #df3b2f;
$color-instagram: #d93174;
$color-eventbrite: #ff8000;
$color-stumbleupon: #eb4924;
$color-reddit: #5f99cf;
$color-pinterest: #cc2127;
$color-pocket: #ee4256;
$color-email: #f8f8f8;
$color-print: #f8f8f8;
$color-skype: #00AFF0;
$color-telegram: #0088cc;
$color-whatsapp: #43d854;

//Social media colors
$color-facebook: #39579a;
$color-twitter: #55ACEE;
$color-gplus: #df4a32;
$color-tumblr: #35465c;
$color-linkedin: #0976b4;
$color-path: #df3b2f;
$color-instagram: #d93174;
$color-eventbrite: #ff8000;
$color-stumbleupon: #eb4924;
$color-reddit: #5f99cf;
$color-pinterest: #cc2127;
$color-pocket: #ee4256;
$color-email: #f8f8f8;
$color-print: #f8f8f8;
$color-skype: #00AFF0;
$color-telegram: #0088cc;
$color-whatsapp: #43d854;
// Uncommon
$border-ultra-light-gray: #e8f0f5;

// Link hovers
$link-highlight: tint($blue-medium, 20%);
97 changes: 97 additions & 0 deletions assets/stylesheets/shared/_themes.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
// ==========================================================================
// Themes
//
// Theming variables and color scheme definitions
//
// 1.0 Theme variables
// 2.0 Color scheme definitions
// 3.0 Theming classes
//
// Usage:
//
// Add a new theme variable:
// I.) Add a new theme variable to section 1.0 of this file
// II.) Add new custom property matching the theme variable name to $color-schemes > default
// III.) Add the same custom property to each theme in $color-schemes
//
// Add a new theme:
// I.) Add a new block to $color-schemes by copying $color-schemes > default
// II.) Change name of new color scheme to desired name for the new theme
// III.) Change color values assigned to the custom properties of the new theme
// ==========================================================================

/**
* 1.0 Theme variables
*
* Declare theme variables and assign default colors
*/

// Layout
$masterbar-color: $white;
$masterbar-bg-color: $blue-wordpress;
$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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?


$sidebar-bg-color: $gray-lighten-30;
$sidebar-text-color: $gray-dark;
$sidebar-selected-color: $gray-text-min;

/**
* 2.0 Color scheme definitions
*
* Assign color variables to custom properties
* Note: theme variable names have to match custom property names in 'default' theme
*/

$color-schemes: (
default: (
'--masterbar-color': $masterbar-color,
'--masterbar-bg-color': $masterbar-bg-color,
'--masterbar-border-color': $masterbar-border-color,
'--masterbar-item-active-bg-color': $masterbar-item-active-bg-color,
'--masterbar-item-hover-bg-color': $masterbar-item-hover-bg-color,
'--masterbar-item-new-color': $masterbar-item-new-color,
),
light: (
'--masterbar-color': $gray-text,
'--masterbar-bg-color': $gray-lighten-20,
'--masterbar-border-color': $gray-lighten-10,
'--masterbar-item-active-bg-color': $gray-lighten-10,
'--masterbar-item-hover-bg-color': $gray-lighten-30,
'--masterbar-item-new-color': $gray-text,
),
dark: (
'--masterbar-color': $masterbar-color,
'--masterbar-bg-color': $gray-dark,
'--masterbar-border-color': $gray-darken-10,
'--masterbar-item-active-bg-color': $gray-text-min,
'--masterbar-item-hover-bg-color': $gray-darken-10,
'--masterbar-item-new-color': $masterbar-item-new-color,
)
);

/**
* 3.0 Theming classes
*
* Assign values for themes at :root level
* This enables us to change the theme via a single class
*/

:root {
@each $color-scheme, $color-scheme-values in $color-schemes {
@if $color-scheme == 'default' {
@each $custom-property, $sass-variable in map-get( $color-schemes, 'default' ) {
#{ $custom-property } : $sass-variable;
}
}
@else {
&.is-#{$color-scheme}-theme {
@each $custom-property, $sass-variable in map-get( $color-schemes, $color-scheme ) {
#{ $custom-property } : $sass-variable;
}
}
}
}
}
1 change: 1 addition & 0 deletions assets/stylesheets/shared/functions/_functions.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
@import 'map-deep-get';
@import 'z-index';
20 changes: 20 additions & 0 deletions assets/stylesheets/shared/functions/_map-deep-get.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@

// ==========================================================================
// Map deep get
//
// Allows us to get values from nested maps.
//
// Usage:
// map-deep-get( $map, $keys... );
// ==========================================================================

@function map-deep-get( $map, $keys... ) {
@each $key in $keys {
@if not map-has-key( $map, $key) {
@warn "No item found for `#{$key}` of `[#{ $keys }]` in provided map. Property omitted.";
@return map-get( $map, $key );
}
$map: map-get( $map, $key );
}
@return $map;
}
12 changes: 0 additions & 12 deletions assets/stylesheets/shared/functions/_z-index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,18 +259,6 @@ $z-layers: (
)
);

// allows us to do a nested fetch
@function map-deep-get( $map, $keys... ) {
@each $key in $keys {
@if not map-has-key( $map, $key) {
@warn "No layer found for `#{$key}` of `[#{ $keys }]` in $z-layers map. Property omitted.";
@return map-get( $map, $key );
}
$map: map-get( $map, $key );
}
@return $map;
}

@function z-index( $keys... ) {
@return map-deep-get( $z-layers, $keys... );
}
34 changes: 34 additions & 0 deletions assets/stylesheets/shared/mixins/_custom-properties.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// ==========================================================================
// Custom properties mixin
//
// Mixins for using custom properties and providing fallbacks
//
// Usage:
// @include custom-property( color, --text-color, $text-color );
// @include custom-property-from-map( color, --text-color, $text-color );
// @include theme( color, --text-color );
// @include theme-multiple(
// ('color': --text-color, 'background': --bg-color, 'border-color': --border-color);
// )
// ==========================================================================

@mixin custom-property( $property, $custom-property, $fallback ) {
#{ $property }: $fallback;
#{ $property }: var( $custom-property, $fallback );
}

@mixin custom-property-from-map( $property, $custom-property, $map, $keys... ) {
#{ $property }: map-deep-get( $map, $keys... );
#{ $property }: var( $custom-property, map-deep-get( $map, $keys... ) );
}

// custom mixins for theming with custom properties based on $color-schemes map (see _themes.scss)
@mixin theme( $property, $custom-property ) {
@include custom-property-from-map( $property, $custom-property, $color-schemes, 'default', $custom-property );
}

@mixin theme-multiple ($theme-properties) {
@each $property, $custom-property in $theme-properties {
@include theme( $property, $custom-property );
}
}
1 change: 1 addition & 0 deletions assets/stylesheets/shared/mixins/_mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
@import 'calc';
@import 'clear-fix';
@import 'clear-text';
@import 'custom-properties';
@import 'dropdown-menu';
@import 'heading';
@import 'hide-content-accessibly';
Expand Down
1 change: 1 addition & 0 deletions assets/stylesheets/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
@import 'shared/functions'; // functions that we've used from Compass, ported over
@import 'shared/functions/functions'; // sass functions for z-index, etc.
@import 'shared/colors'; // import all of our wpcom colors
@import 'shared/themes'; // theming variables and color scheme definitions
@import 'shared/typography'; // all the typographic rules, variables, etc.
@import 'shared/mixins/mixins'; // sass mixins for gradients, bordius radii, etc.
@import 'shared/extends'; // sass extends for commonly used styles
Expand Down
4 changes: 2 additions & 2 deletions client/components/sub-masterbar-nav/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
display: flex;
flex-direction: column;
padding: 56px 20px 28px;
background-color: $masterbar-color;
background-color: $masterbar-bg-color;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@frontdevde frontdevde Jul 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.


&:not(.is-collapsed) .sub-masterbar-nav__select {
background-color: $blue-dark;
Expand Down Expand Up @@ -89,7 +89,7 @@

&:hover,
&:focus {
background: lighten( $masterbar-color, 5% );
background: lighten( $masterbar-bg-color, 5% );
color: $white;
outline: 0;
transition: all 200ms ease-out;
Expand Down
Loading