Colour Scheming: Update External Brand Variables #30084

Aurorum · 2019-01-10T11:05:06Z

Changes proposed in this Pull Request

Follow up to #30055, this PR will update external brand colours (all but Jetpack and WP) with variables. There should be no visual changes.

Edit: Also take the opportunity to remove Path colours, since it closed down last year and is no longer used throughout Calypso (see #27252)

Testing instructions

Is everything valid? Were any typos made (eg. with HEX codes)? Are there any remaining variables which need to be changed but haven't be touched yet?

Aurorum · 2019-01-10T13:43:46Z

Okay, I think that's the last of them. 👍 Still not familiar with squashing yet, but I'm hoping/assuming that can be done by whoever merges. Added testing instructions above! :)

(cc @flootr)

Aurorum · 2019-01-10T21:52:08Z

client/gutenberg/extensions/publicize/editor.scss

 	}
 	&.is-google-plus {
-		fill: #df4a33;
+		fill: var( --color-gplus );


I noticed that these use HEX codes...is there a specific reason why? I'm assuming it can just be set to variables too.
(cc @floor, @blowery, @drw158)

Agreed. Are all the hex values the same?

These ones are, but the ones here are a bit different. The HEX code seem are identical visually to the actual social media icons though, so I'm assuming they also should use a variable.

Make sure this works it IE11 and that the fallback hex codes get properly inserted.

assets/stylesheets/shared/_color-schemes.scss

flootr · 2019-01-16T07:28:46Z

client/my-sites/sharing/style.scss

@@ -900,24 +900,24 @@
 	}
 }

-@include sharing-button-service( 'facebook', $color-facebook );
-@include sharing-button-service( 'twitter', $color-twitter );
+@include sharing-button-service( 'facebook', var( --color-facebook ));


I think those won't work. The sharing-button-service passes those values to an rgb( ... ) function which doesn't know how to deal with hex values. The way we work around this is using the hex-to-rgb sass function defined here. You can see a usage example here.

You could either define a -rgb version of each CSS custom prop or use the hex-to-rgb( ... ) function directly at this place. As far as I can see we don't pass those color variables anywhere else to an rgba( .... ) function so the latter is probably fine.

Let me know if that makes sense to you.

I think so, but just to confirm. Something like this?

Suggested change

@include sharing-button-service( 'facebook', var( --color-facebook ));

@include sharing-button-service( 'facebook', rgba(var( --color-facebook )));

That looks like a confusing number of brackets though and just doesn't look right, so just want to be sure.

Sorry, I think it would've been more clear from my side if I provided a code example. What I meant is the following:

/* _color-schemes.scss */ --color-facebook: #{$color-facebook}; --color-facebook-rgb: #{hex-to-rgb( $color-facebook )}; // this file @include sharing-button-service( 'facebook', var( --color-facebook-rgb ));

This is necessary because the sharing-button-service mixin is passing it to rgba( ... ) here:

wp-calypso/client/my-sites/sharing/style.scss

Lines 897 to 899 in 2d50a28

&:hover {

background: rgba( $color, 0.9 );

}

and the above rgba function doesn't work with hex values

Thanks, that helps a ton! Just one more question, since $color-facebook would otherwise be removed, should they use HEX codes instead?

--color-facebook-rgb: #{hex-to-rgb( #39579a )};

I think that'd work. Only notice that we need both --color-facebook and --color-facebook-rgb so there's some repetition but I think that may be fine in this case.

Thank you! I've added a commit which intends to address this. Do you know if that's the same case here too?

The color values are directly used in the mentioned mixin so we can use the normal variables (without the -rgb suffix) there.

flootr

Found one last thing. Apart from that this should be ready to go 👍

flootr · 2019-01-17T14:05:46Z

client/my-sites/sharing/style.scss

-@include sharing-button-service( 'facebook', $color-facebook );
-@include sharing-button-service( 'twitter', $color-twitter );
+@include sharing-button-service( 'facebook', var( --color-facebook-rgb ));
+@include sharing-button-service( 'twitter', var( --color-twitter-rgb ));
 @include sharing-button-service( 'press-this', $blue-wordpress );


this could use var( --color-primary-rgb ) instead of $blue-wordpress, correct?

Yep, added a commit for that. :)

@torres126 Press This is a WordPress brand so it should use the new --color-wpcom variable, please. In other words, we don't want the color to change with schemes, which --color-primary might do.

Why are these rgb by the way?

Oh I see you all discussed it above.

davewhitley · 2019-01-17T16:19:39Z

Sorry to come in late on this! I think we can eliminate the need for the rgb variables for the brands.

We need to get rid of the sharing-button-service mixin

wp-calypso/client/my-sites/sharing/style.scss

Lines 897 to 899 in 2d50a28

    
           &:hover { 
        
           	background: rgba( $color, 0.9 ); 
        
           }

I don't think it's doing much visually. You can see that there's already an opacity change on the container div — we don't need more opacity on the color:

Aurorum · 2019-01-17T16:23:48Z

Oh, oops, I added a commit to start on the Press This issue right as you posted that. 😅

So just to confirm here, what needs to be got rid of? Just keep lines 897-899, and then using the normal variables on 903-920, before removing the RGB variables added earlier?

davewhitley · 2019-01-17T16:31:05Z

assets/stylesheets/shared/_color-schemes.scss

-	--color-whatsapp: #{$color-whatsapp};
-	--color-wordpress-org: #{$color-wordpressOrg};
+	--color-email: #f8f8f8;
+	--color-email-rgb: #{hex-to-rgb( #f8f8f8 )};	


We don't need any of these rgb variables

Removed. 👍

davewhitley · 2019-01-17T16:31:36Z

client/my-sites/sharing/style.scss

-@include sharing-button-service( 'skype', $color-skype );
-@include sharing-button-service( 'telegram', $color-telegram );
-@include sharing-button-service( 'jetpack-whatsapp', $color-whatsapp );
+@include sharing-button-service( 'facebook', var( --color-facebook-rgb ));


Edit: sorry, instead of removing these references, let's simplify the mixin found here:

wp-calypso/client/my-sites/sharing/style.scss

Line 897 in 2d50a28

&:hover {

To just be:

@mixin sharing-button-service( $name, $color ) { .sharing-buttons-preview-button.style-icon.share-#{ $name } { background: $color; &:hover { background: $color; } } }

By doing that, you won't need any rgb variables.

davewhitley · 2019-01-17T16:38:32Z

@torres126 List of changes:

Simplify this mixin so that it doesn't need rgb:

wp-calypso/client/my-sites/sharing/style.scss

Line 897 in 2d50a28

&:hover {

See my comment here
Get rid of the rgb variables

I believe that is what needs to be done. After that, we can wait for a final review from a developer. Thanks!

Aurorum · 2019-01-17T16:41:53Z

Thanks @drw158! I just added two commits to address this, do they help deal with this?

davewhitley · 2019-01-17T16:48:17Z

client/my-sites/sharing/style.scss

+@include sharing-button-service( 'facebook', var( --color-facebook ));
+@include sharing-button-service( 'twitter', var( --color-twitter ));
+@include sharing-button-service( 'press-this', var( --color-wpcom ));
+@include sharing-button-service( 'path', var( --color-path-rgb ));


Seems to be an rgb here?

Oops, removed.

Sidenote: I've not checked if it's used across Calypso yet, but does Path even need to be included at all? It closed down a while ago (see #27252).

The color is still being used here

wp-calypso/client/blocks/post-share/style.scss

Line 241 in 006d96c

color: $color-path;

But I don't think it's an option anymore, so probably fine to remove

flootr

What a PR! Thanks for working on this @torres126 , it's highly appreciated 👍 let's ship this now

Aurorum · 2019-01-18T08:15:22Z

Thanks @flootr! :)

Aurorum added 4 commits January 10, 2019 08:24

Replace External Brand Colours

761318e

Replace Brand Colour Values

ded970e

Replace Brand Colour Values

14a8074

Replace Brand Colour Values

64ab268

Aurorum mentioned this pull request Jan 10, 2019

Colour Scheming: Add Brand Colours #30055

Merged

flootr added the Color Schemes label Jan 10, 2019

Aurorum added 4 commits January 10, 2019 13:10

Replace with HEX

4d926d2

Remove Social Media Colours

be256cd

Update Colour Variable

f098ba2

Update Brand Colour Variables

4e1f001

Replace HEX with Variables

4bcd76c

Aurorum commented Jan 10, 2019

View reviewed changes

Replace HEX With Variables

61a7d17

flootr added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 11, 2019

flootr requested a review from a team January 11, 2019 07:05

flootr reviewed Jan 11, 2019

View reviewed changes

assets/stylesheets/shared/_color-schemes.scss Outdated Show resolved Hide resolved

Alphabeticise Variables

2d50a28

flootr reviewed Jan 16, 2019

View reviewed changes

Aurorum added 2 commits January 17, 2019 13:32

Add RGB Values

78c91eb

Use RGB Instead of HEX

4db6d98

flootr reviewed Jan 17, 2019

View reviewed changes

Update "Press This" Variable

a32fb17

davewhitley self-assigned this Jan 17, 2019

Add WPcom RGB

c10ecb9

davewhitley reviewed Jan 17, 2019

View reviewed changes

Remove Social Brand RGB Values

8580748

davewhitley removed their assignment Jan 17, 2019

Remove RGB + Simplify mixin

3eae19a

davewhitley reviewed Jan 17, 2019

View reviewed changes

Aurorum added 4 commits January 17, 2019 16:53

Remove Path RGB

359ef38

Remove Path

82d8a29

Remove Path

58550d1

Remove Path Variable

af39539

flootr approved these changes Jan 18, 2019

View reviewed changes

flootr added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 18, 2019

flootr merged commit a71dec9 into Automattic:master Jan 18, 2019

matticbot removed the [Status] Ready to Merge label Jan 18, 2019

Aurorum mentioned this pull request Jan 18, 2019

Colour Scheming: Replace Jetpack Variable #30232

Merged

Aurorum mentioned this pull request Jan 29, 2019

Partially revert Jetpack colour variable change for Gutenberg extensions #30468

Merged

arunsathiya added the OSS Citizen label Feb 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colour Scheming: Update External Brand Variables #30084

Colour Scheming: Update External Brand Variables #30084

Aurorum commented Jan 10, 2019 •

edited

Loading

Aurorum commented Jan 10, 2019

Aurorum Jan 10, 2019

flootr Jan 11, 2019

Aurorum Jan 11, 2019

davewhitley Jan 17, 2019

flootr Jan 16, 2019

Aurorum Jan 16, 2019

flootr Jan 16, 2019 •

edited

Loading

Aurorum Jan 16, 2019

flootr Jan 17, 2019

Aurorum Jan 17, 2019

flootr Jan 17, 2019

flootr left a comment

flootr Jan 17, 2019

Aurorum Jan 17, 2019

davewhitley Jan 17, 2019

davewhitley Jan 17, 2019

davewhitley commented Jan 17, 2019

Aurorum commented Jan 17, 2019 •

edited

Loading

davewhitley Jan 17, 2019

Aurorum Jan 17, 2019

davewhitley Jan 17, 2019 •

edited

Loading

davewhitley commented Jan 17, 2019 •

edited

Loading

Aurorum commented Jan 17, 2019

davewhitley Jan 17, 2019

Aurorum Jan 17, 2019 •

edited

Loading

davewhitley Jan 17, 2019

flootr left a comment

Aurorum commented Jan 18, 2019

	@include sharing-button-service( 'facebook', var( --color-facebook ));
	@include sharing-button-service( 'facebook', rgba(var( --color-facebook )));

Colour Scheming: Update External Brand Variables #30084

Colour Scheming: Update External Brand Variables #30084

Conversation

Aurorum commented Jan 10, 2019 • edited Loading

Changes proposed in this Pull Request

Testing instructions

Aurorum commented Jan 10, 2019

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

flootr Jan 16, 2019 • 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

flootr 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

Choose a reason for hiding this comment

davewhitley commented Jan 17, 2019

Aurorum commented Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davewhitley Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

davewhitley commented Jan 17, 2019 • edited Loading

Aurorum commented Jan 17, 2019

Choose a reason for hiding this comment

Aurorum Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flootr left a comment

Choose a reason for hiding this comment

Aurorum commented Jan 18, 2019

Aurorum commented Jan 10, 2019 •

edited

Loading

flootr Jan 16, 2019 •

edited

Loading

Aurorum commented Jan 17, 2019 •

edited

Loading

davewhitley Jan 17, 2019 •

edited

Loading

davewhitley commented Jan 17, 2019 •

edited

Loading

Aurorum Jan 17, 2019 •

edited

Loading