Colour Scheming: Replace Jetpack Variable #30232

Aurorum · 2019-01-18T08:53:27Z

Changes proposed in this Pull Request

Follow up to #30084, this PR should update most brand colours with variables. I've not removed $green-jetpack completely yet because there's a couple of cases where I'm not sure which variable to use due to the lighten/darken effect, and I don't think variables will work there? This PR addresses all the other ones though.

Testing instructions

There should be no visible changes, so does the code make sense?

Edit: Minor visible changes due to the darken/lighten variables being replaced

(cc @flootr, @drw158, @blowery)

flootr

Thanks for working on this! While this looks good to me (👍) we should clarify your question on how to address cases where we, at the moment, lighten or darken those brand colors.

flootr · 2019-01-18T09:22:13Z

client/blocks/login/style.scss

@@ -2,7 +2,7 @@
 .login {
 	&.is-jetpack {
 		.button.is-primary {
-			background-color: $green-jetpack;
+			background-color: var( --color-jetpack );
 			border-color: darken( $green-jetpack, 5% );


@drw158 do you have feedback on how to address those? I'm not sure how I feel about lighten or darken brand colors but if we do so I guess we should pre-define them as the others, like --color-jetpack-light or --color-jetpack-dark.

There's three different ones in total, btw:

darken( $green-jetpack, 30% );
darken( $green-jetpack, 5% );
lighten( $green-jetpack, 25% );

@jeffgolenski We are replacing all scss variables with css custom props. I'm wondering if Jetpack has official shades of the Jetpack Green, or is it ok to generate a couple of shades for the purposes of the buttons in Calypso?

@drw158 We only have one "official" jetpack green now. Feel free to explore some different shades as you see fit. the light and darken colors mentioned above aren't really official, but we use them in a couple places.

If this is confined to just the button styles, I think defining dark and light colors for Jetpack is fine (-color-jetpack-light or --color-jetpack-dark). Jetpack is a unique case because its color is not from the Color Studio palette and thus doesn't have 10 shades like the other colors. Jetpack green is also unique because it needs more shades to create button styles.

Let's use #81d677 for light and #2e8928 for dark

Aurorum · 2019-01-22T18:24:11Z

client/jetpack-connect/jetpack-new-site/style.scss

 		}

 		&:focus {
-			box-shadow: 0 0 0 2px lighten( $green-jetpack, 25% );
+			box-shadow: 0 0 0 2px var( --color-jetpack-light );
 		}

 		&[disabled],
 		&:disabled {
 			background: tint( $green-jetpack, 50% );
 			border-color: tint( $green-jetpack, 35% );


I'm not feeling too confident on if variables work here. If they don't, I'm a bit stumped on which to use for there to be a visible difference.

We recently aligned disabled buttons to look the same across button types. I think that should be the same here as well. Compare with #30229.

Thanks, added!

flootr · 2019-01-23T09:51:55Z

It seems like this branch doesn't build at the moment. The file client/jetpack-connect/style.scss tries to use the $green-jetpack variable which is removed. Would you mind looking into that?

Here's the complete error message (for npm start):

{
  "status": 1,
  "file": "client/jetpack-connect/style.scss",
  "line": 43,
  "column": 22,
  "message": "Undefined variable: \"$green-jetpack\".",
  "formatted": "Error: Undefined variable: \"$green-jetpack\".\n        on line 43 of client/jetpack-connect/style.scss\n        from line 201 of assets/stylesheets/_components.scss\n        from line 17 of assets/stylesheets/style.scss\n>> \t\t\tbackground: tint( $green-jetpack, 50% );\n\n   ---------------------^\n"
}
{
  "status": 1,
  "file": "client/jetpack-connect/style.scss",
  "line": 43,
  "column": 22,
  "message": "Undefined variable: \"$green-jetpack\".",
  "formatted": "Error: Undefined variable: \"$green-jetpack\".\n        on line 43 of client/jetpack-connect/style.scss\n        from line 201 of assets/stylesheets/_components.scss\n        from line 17 of assets/stylesheets/style.scss\n>> \t\t\tbackground: tint( $green-jetpack, 50% );\n\n   ---------------------^\n"
}
{
  "status": 1,
  "file": "client/jetpack-connect/style.scss",
  "line": 43,
  "column": 22,
  "message": "Undefined variable: \"$green-jetpack\".",
  "formatted": "Error: Undefined variable: \"$green-jetpack\".\n        on line 43 of client/jetpack-connect/style.scss\n        from line 201 of assets/stylesheets/_components.scss\n        from line 17 of assets/stylesheets/style.scss\n        from line 3 of assets/stylesheets/sections/devdocs.scss\n>> \t\t\tbackground: tint( $green-jetpack, 50% );\n\n   ---------------------^\n"
}

Aurorum · 2019-01-23T11:08:12Z

Sorry, forgot about those, switched now.

flootr

I think this should be good to go after addressing my comments.

In the long term we should use the actual <Button /> component and add Jetpack styles (is-jetpack?) there. I can see at least two different overrides while looking through the code. Not in the scope of this PR though.

client/jetpack-connect/jetpack-new-site/style.scss

client/jetpack-connect/style.scss

flootr · 2019-01-23T14:27:15Z

client/jetpack-connect/style.scss

-		background-color: $green-jetpack;
-		border-color: darken( $green-jetpack, 5% );
+		background-color: var( --color-jetpack );
+		border-color: var( --color-jetpack-light );


this should be --color-jetpack-dark

My concern with changing these lights to dark would mean that the hover state wouldn't have a visible effect. Is it okay to lose that?

My concern with changing these lights to dark would mean that the hover state wouldn't have a visible effect. Is it okay to lose that?

That concern is right 👍

The <Button /> component uses a lighter background color for hover states which I think would be appropriate here too. You could add (new) hover styles to work around this, though I think we then should rather use the actual <Button /> component instead of recreating it - the latter fits in the long term "vision" I mentioned in #30232 (review).

I've switched all the ones that you recommended to be dark.

You could add (new) hover styles to work around this

So I understand correctly here, do you suggest adding a new variable, perhaps --color-jetpack-hover, so that there's a difference in hover styles currently?

.button.is-primary { background-color: var( --color-jetpack ); border-color: var( --color-jetpack-dark ); &:hover, &:focus { border-color: var( --color-jetpack-dark ); }`

Just to clarify, my concern is that the hover border will be identical. From a really brief look, I don't think there's too a major issue with background colours on hover though.

In the long term we should use the actual component and add Jetpack styles (is-jetpack?) there.

Agreed, happy to take a stab at that after this!

So to just clarify, is this correct?

Background colours:

Normal state - Jetpack green
Hover/Focus state - Lighter Jetpack

Border colours:

Normal state - Dark Jetpack
Hover/Focus state - This PR currently proposes also Dark Jetpack, which seems off...but I think changing that would require a new variable. Perhaps --color-jetpack-hover at #002508 which seems to be the one now, but I'm not certain about this.

For reference, here's the current styles.

My suggestion is to align it with the current <Button /> implementation. Though we might need to introduce another light green color for hover states.

That's what the current <Button /> for e.g. a primary one looks like in terms of colors:

normal:

background: --color-accent

border: --color-accent-dark

hover:

background: --color-accent-400 (this one does not currently exist for Jetpack)

border: --color-accent-dark (border color doesn't change for hover)

/cc @drw158

I hope that makes it more clear. Let me know if you have further questions and I'll try to answer them accordingly.

400 in Jetpack Green would be #57ca53

Let me know if you need any more!

So currently, --color-jetpack-light is being used five times, and only for hover or focus button styles, either as the background-color or the box-shadow. In that case, should the current #81d677 be replaced with #57ca53?

--color-jetpack-light is not the same as --color-jetpack-400 (we need to define the latter separately with the value #57ca53 from #30232 (comment)

In that case, should the current #81d677 be replaced with #57ca53?

The latter should be used for the button background color on hover.

Thanks for confirming, just added some commits, let me know if that's what you're referring to. :)

flootr

LGTM

This partially reverts commit 3f05571.

ockham · 2019-01-29T16:21:31Z

client/gutenberg/extensions/presets/jetpack/editor-shared/jetpack-plugin-sidebar.scss

@@ -29,11 +29,11 @@
 			}

 			.jetpack-logo__icon-circle {
-				fill: $green-jetpack !important;
+				fill: var( --color-jetpack ) !important;


This caused #30469

…ons (#30468) * Partially revert "Colour Scheming: Replace Jetpack Variable (#30232)" This partially reverts commit 3f05571.

Aurorum added 7 commits January 18, 2019 08:37

Update Colour Variables

f682657

Update Colour Variable

d5b9935

Update Colour Variable

0ced895

Update Colour Variable

2065546

Replace Colour Variable

7ed95d0

Replace Colour Variable

bf78928

Replace Jetpack Variable

16ed51d

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

flootr requested review from davewhitley and a team January 18, 2019 09:19

flootr reviewed Jan 18, 2019

View reviewed changes

Aurorum added 4 commits January 22, 2019 18:12

Add other Jetpack variables

08dec35

Remove Jetpack

355ee8c

Use Variable

a96b6d5

Update Variables

8de40ba

Aurorum changed the title ~~Colour Scheming: Replace Jetpack Variable~~ Colour Scheming: Replace Jetpack Variable (WIP) Jan 22, 2019

Use Variable

9968471

Aurorum commented Jan 22, 2019

View reviewed changes

Aurorum changed the title ~~Colour Scheming: Replace Jetpack Variable (WIP)~~ Colour Scheming: Replace Jetpack Variable Jan 22, 2019

Disabled Styles

d61ea78

flootr requested a review from a team January 23, 2019 08:30

Switch Other Variables

d659ebf

Aurorum added 2 commits January 23, 2019 13:43

Last switch 🤞

097b417

Missed one more...

53791cc

flootr reviewed Jan 23, 2019

View reviewed changes

Aurorum added 2 commits January 23, 2019 17:31

Switch Variable

c29747f

Switch Variable

2e7c7dc

Aurorum added 6 commits January 23, 2019 17:33

Switch Variable

e7c9659

Switch Hover Variables

b0c3efd

Switch Hover Variable

a145559

Define Jetpack 400

916b01b

Hover Background Colour

c56ec7c

Hover Background Colour

ba14acc

flootr approved these changes Jan 24, 2019

View reviewed changes

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

flootr merged commit 3f05571 into Automattic:master Jan 24, 2019

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

Aurorum mentioned this pull request Jan 24, 2019

Colour Scheming: Try Adding Jetpack Button (WIP) #30387

Closed

simison added a commit that referenced this pull request Jan 29, 2019

Partially revert "Colour Scheming: Replace Jetpack Variable (#30232)"

511aba4

This partially reverts commit 3f05571.

simison mentioned this pull request Jan 29, 2019

Partially revert Jetpack colour variable change for Gutenberg extensions #30468

Merged

ockham reviewed Jan 29, 2019

View reviewed changes

simison added a commit that referenced this pull request Jan 29, 2019

Partially revert Jetpack colour variable change for Gutenberg extensi…

5e7a079

…ons (#30468) * Partially revert "Colour Scheming: Replace Jetpack Variable (#30232)" This partially reverts commit 3f05571.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colour Scheming: Replace Jetpack Variable #30232

Colour Scheming: Replace Jetpack Variable #30232

Aurorum commented Jan 18, 2019 •

edited

Loading

flootr left a comment

flootr Jan 18, 2019

Aurorum Jan 18, 2019

davewhitley Jan 18, 2019

jeffgolenski Jan 18, 2019

davewhitley Jan 22, 2019

Aurorum Jan 22, 2019

flootr Jan 23, 2019

Aurorum Jan 23, 2019

flootr commented Jan 23, 2019

Aurorum commented Jan 23, 2019

flootr left a comment

flootr Jan 23, 2019

Aurorum Jan 23, 2019

flootr Jan 23, 2019

Aurorum Jan 23, 2019

flootr Jan 23, 2019

davewhitley Jan 23, 2019

Aurorum Jan 23, 2019

flootr Jan 24, 2019

flootr Jan 24, 2019

Aurorum Jan 24, 2019

flootr left a comment

ockham Jan 29, 2019

Colour Scheming: Replace Jetpack Variable #30232

Colour Scheming: Replace Jetpack Variable #30232

Conversation

Aurorum commented Jan 18, 2019 • edited Loading

Changes proposed in this Pull Request

Testing instructions

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

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 commented Jan 23, 2019

Aurorum commented Jan 23, 2019

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aurorum commented Jan 18, 2019 •

edited

Loading