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

Colour Scheming: Replace Jetpack Variable #30232

Merged
merged 24 commits into from
Jan 24, 2019
Merged

Colour Scheming: Replace Jetpack Variable #30232

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
f682657
Update Colour Variables
Aurorum Jan 18, 2019
d5b9935
Update Colour Variable
Aurorum Jan 18, 2019
0ced895
Update Colour Variable
Aurorum Jan 18, 2019
2065546
Update Colour Variable
Aurorum Jan 18, 2019
7ed95d0
Replace Colour Variable
Aurorum Jan 18, 2019
bf78928
Replace Colour Variable
Aurorum Jan 18, 2019
16ed51d
Replace Jetpack Variable
Aurorum Jan 18, 2019
08dec35
Add other Jetpack variables
Aurorum Jan 22, 2019
355ee8c
Remove Jetpack
Aurorum Jan 22, 2019
a96b6d5
Use Variable
Aurorum Jan 22, 2019
8de40ba
Update Variables
Aurorum Jan 22, 2019
9968471
Use Variable
Aurorum Jan 22, 2019
d61ea78
Disabled Styles
Aurorum Jan 23, 2019
d659ebf
Switch Other Variables
Aurorum Jan 23, 2019
097b417
Last switch 🤞
Aurorum Jan 23, 2019
53791cc
Missed one more...
Aurorum Jan 23, 2019
c29747f
Switch Variable
Aurorum Jan 23, 2019
2e7c7dc
Switch Variable
Aurorum Jan 23, 2019
e7c9659
Switch Variable
Aurorum Jan 23, 2019
b0c3efd
Switch Hover Variables
Aurorum Jan 23, 2019
a145559
Switch Hover Variable
Aurorum Jan 23, 2019
916b01b
Define Jetpack 400
Aurorum Jan 24, 2019
c56ec7c
Hover Background Colour
Aurorum Jan 24, 2019
ba14acc
Hover Background Colour
Aurorum Jan 24, 2019
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
5 changes: 4 additions & 1 deletion assets/stylesheets/shared/_color-schemes.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,10 @@
--color-link-900-rgb: #{hex-to-rgb( $muriel-blue-900 )};

--color-wpcom: #{$muriel-blue-500};
--color-jetpack: #{$green-jetpack};
--color-jetpack: #00be28;
--color-jetpack-light: #81d677;
--color-jetpack-dark: #2e8928;

--color-email: #f8f8f8;
--color-eventbrite: #ff8000;
--color-facebook: #39579a;
Expand Down
3 changes: 0 additions & 3 deletions assets/stylesheets/shared/_colors.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ $blue-light: $muriel-blue-300;
$blue-medium: $muriel-blue-500;
$blue-dark: $muriel-blue-700;

// Jetpack Green
$green-jetpack: #00be28;

// Grays
$gray: $muriel-gray-300;
$gray-light: $muriel-gray-0;
Expand Down
4 changes: 2 additions & 2 deletions client/blocks/login/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
.login {
&.is-jetpack {
.button.is-primary {
background-color: $green-jetpack;
border-color: darken( $green-jetpack, 5% );
background-color: var( --color-jetpack );
border-color: var( --color-jetpack-dark );
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion client/components/upgrades/gsuite/gsuite-dialog/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
}

strong {
color: $green-jetpack;
color: var( --color-jetpack );
margin-left: 0.3em;

@include breakpoint( '<480px' ) {
Expand Down
4 changes: 2 additions & 2 deletions client/gutenberg/extensions/presets/jetpack/editor-shared/jetpack-plugin-sidebar.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@
}

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

Choose a reason for hiding this comment

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

This caused #30469

}

.jetpack-logo__icon-triangle {
fill: $white !important;
fill: var( --color-white ) !important;
}
}
}
Expand Down
20 changes: 10 additions & 10 deletions client/jetpack-connect/jetpack-new-site/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
}

.jetpack-new-site__jetpack-site .jetpack-logo {
fill: $green-jetpack;
fill: var( --color-jetpack );
}

.jetpack-new-site__card-title {
Expand Down Expand Up @@ -130,31 +130,31 @@

input[type='text'] {
&:focus {
border-color: $green-jetpack;
box-shadow: 0 0 0 2px lighten( $green-jetpack, 25% );
border-color: var( --color-jetpack );
box-shadow: 0 0 0 2px var( --color-jetpack-light );
}
}

.button {
background-color: $green-jetpack;
border-color: darken( $green-jetpack, 5% );
background-color: var( --color-jetpack );
border-color: var( --color-jetpack-light );
Aurorum marked this conversation as resolved.
Show resolved Hide resolved
margin-top: 16px;
width: 100%;

&:hover,
&:focus {
border-color: darken( $green-jetpack, 30% );
border-color: var( --color-jetpack-dark );
}

&: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% );
color: $white;
background-color: var( --color-white );
border-color: var( --color-neutral-50 );
color: var( --color-neutral-50 );
}
}
}
Expand Down
30 changes: 15 additions & 15 deletions client/jetpack-connect/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,23 @@
}

.button.is-primary {
background-color: $green-jetpack;
border-color: darken( $green-jetpack, 5% );
background-color: var( --color-jetpack );
border-color: var( --color-jetpack-light );
Aurorum marked this conversation as resolved.
Show resolved Hide resolved

&:hover,
&:focus {
border-color: darken( $green-jetpack, 30% );
border-color: var( --color-jetpack-dark );
}

&: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% );
color: $white;
background: var( --color-white );
border-color: var( --color-neutral-50);
color: var( --color-neutral-50 );
}
}
}
Expand Down Expand Up @@ -231,7 +231,7 @@
.example-components__install-plugin-header {
width: 100%;
padding-bottom: 12%;
background-color: $green-jetpack;
background-color: var( --color-jetpack );
}

.example-components__install-plugin-body {
Expand All @@ -254,7 +254,7 @@
font-size: 12px;
padding: 7px 14px;
line-height: 1;
color: $white;
color: var( --color-white );
background-color: #008ec2; // wp-admin button blue
border: 1px solid #006799;
border-radius: 3px;
Expand Down Expand Up @@ -298,15 +298,15 @@
.example-components__content-wp-admin-connect-banner {
margin: 10px;
padding: 10px 10px 12px;
background-color: $white;
background-color: var( --color-white );
}

.example-components__content-wp-admin-connect-button {
display: inline-block;
font-size: 12px;
padding: 7px 14px;
line-height: 1;
color: $white;
color: var( --color-white );
border-radius: 3px;
background-color: #00be28;
border: 2px #00a523 solid;
Expand All @@ -328,7 +328,7 @@
font-size: 12px;
line-height: 1;
padding: 11px 10px;
color: $white;
color: var( --color-white );
background-color: #518d2a;
border-color: #518d2a;
border-radius: 4px;
Expand All @@ -339,7 +339,7 @@
.example-components__content-wp-admin-plugin-card {
margin: 10px;
padding: 12px 10px;
background-color: $white;
background-color: var( --color-white );
}

.example-components__content-wp-admin-plugin-name {
Expand Down Expand Up @@ -622,8 +622,8 @@
}

&.is-primary {
background-color: $green-jetpack;
border-color: darken( $green-jetpack, 5% );
background-color: var( --color-jetpack );
border-color: var( --color-jetpack-light );
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be --color-jetpack-dark

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

fgdsfgdsfdsgfdg

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

400 in Jetpack Green would be #57ca53

Let me know if you need any more!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

--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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}

Expand Down
2 changes: 1 addition & 1 deletion client/my-sites/checkout/concierge-session-nudge/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@

&-item-icon {
width: 18px;
fill: $green-jetpack;
fill: var( --color-jetpack );
margin-right: 5px;
}

Expand Down
2 changes: 1 addition & 1 deletion client/my-sites/feature-upsell/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ $feature-upsell-break-at: '1040px';
&-guarantee {
display: block;
opacity: 0.8;
color: $green-jetpack;
color: var( --color-jetpack );
font-size: 15px;
line-height: 31px;
font-weight: 300;
Expand Down