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

Fix _wp-components-overrides.scss by increasing selector specifity #79708

Closed
wants to merge 9 commits into from
Closed

Fix _wp-components-overrides.scss by increasing selector specifity #79708

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
77ded48
Populate CSS variables to match the design
alshakero Jul 20, 2023
0fd06fd
More general solution
alshakero Jul 21, 2023
d183d2a
Add border-radius to button
alshakero Jul 21, 2023
ffb92f9
Fix comment
alshakero Jul 21, 2023
05ae6dd
Remove extraneous props
alshakero Jul 21, 2023
a7177e3
Fix the root cause
alshakero Jul 21, 2023
6789223
Revert "Fix the root cause"
alshakero Jul 21, 2023
ff5d1f2
Revert "Revert "Fix the root cause""
alshakero Jul 21, 2023
f157e20
Style disabled state in the step
alshakero Jul 21, 2023
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
12 changes: 8 additions & 4 deletions client/assets/stylesheets/_wp-components-overrides.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@
}

/* @wordpress/components Button overrides */
.components-button {
a.components-button,
button.components-button {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like we're missing a root cause here, why is adding this a fix?

As long as we're importing our overrides (this file) after the wordpress styles get imported (@wordpress/components/build-style/style), then we shouldn't need this change right?

I think what's happening is client/assets/stylesheets/style.css is being loaded into the stepper framework, where maybe previously it wasn't?

The client style.css file pulls in these lines:

wp-calypso/client/assets/stylesheets/style.scss

Lines 18 to 21 in a610546

@import "@wordpress/components/build-style/style";
// Overrides for @wordpress/components
@import "./wp-components-overrides";

Then later in the stepper framework pull in wordpress/component styles again here:

wp-calypso/client/landing/stepper/declarative-flow/internals/global.scss

Line 4 in a610546

@import "@wordpress/components/build-style/style";

This overrides our overrides back with original styling.

I think the fix is to remove that line and then add our new overrides as required for wordpress/components in either the calypso-wide override file client/assets/stylesheets/_wp-components-overrides.scss or the stepper framework's client/landing/stepper/declarative-flow/internals/global.scss depending on which is more appropriate for the override.

I confirmed that if you remove that import line in global.scss and then add in the color overrides again:

// WordPress.org components no longer use blue-50 as the primary color.
// This changes the primary color to blue-50 to conform to the WordPress.com colors.
:root {
	--wp-components-color-accent: var(--studio-blue-50);
	--wp-components-color-accent-darker-20: var(--studio-blue-60);
	--wp-components-color-accent-darker-10: var(--studio-blue-60);
}

That the links have the right color in the link-in-bio flow.

We likely still need a new disabled button style added into the calypso-wide overrides file for a complete fix. Commented below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great investigation @lsl, I created a first version of this idea here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Louis! Much appreciated.

&,
&:hover:not(:disabled) {
color: var(--color-neutral-70);
}
}

.components-button.is-primary {
a.components-button.is-primary,
button.components-button.is-primary {
background-color: var(--color-accent);
border-color: var(--color-accent);
color: var(--color-text-inverted);
Expand All @@ -40,7 +42,8 @@
}
}

.components-button.is-secondary {
a.components-button.is-secondary,
button.components-button.is-secondary {
background-color: var(--color-surface);
color: var(--color-neutral-70);
box-shadow: inset 0 0 0 1px var(--color-neutral-10);
Expand All @@ -64,7 +67,8 @@
}
}

.components-button.is-tertiary {
a.components-button.is-tertiary,
button.components-button.is-tertiary {
color: var(--color-accent);

&:active:not(:disabled),
Expand Down
1 change: 1 addition & 0 deletions ...g/stepper/declarative-flow/internals/steps-repository/domain-transfer-domains/domains.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ const Domains: React.FC< Props > = ( { onSubmit } ) => {
<Button
disabled={ numberOfValidDomains === 0 || ! allGood }
className="bulk-domain-transfer__cta"
variant="primary"
onClick={ handleAddTransfer }
>
{ getTransferButtonText() }
Expand Down
11 changes: 6 additions & 5 deletions ...g/stepper/declarative-flow/internals/steps-repository/domain-transfer-domains/styles.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,6 @@
box-shadow: unset;
}

.components-button:active:not(:disabled) {
box-shadow: 0 0 0 var(--wp-admin-border-width-focus) var(--wp-components-color-accent, var(--wp-admin-theme-color, #007cba));
outline: 3px solid transparent;
}

&.domains {
padding: 0;
width: 754px;
Expand Down Expand Up @@ -418,4 +413,10 @@
width: 150px;
@include placeholder();
}

button.components-button.bulk-domain-transfer__cta {
&:disabled {
background: var(--studio-gray-5);
}
}
Comment on lines +417 to +421
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this over to the wp-components-overrides file and generalize for all buttons? or if that's not an option right now at least into the stepper global.scss?

}
2 changes: 1 addition & 1 deletion ...nding/stepper/declarative-flow/internals/steps-repository/domain-transfer-intro/intro.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ const Intro: React.FC< Props > = ( { onSubmit } ) => {
onSelect={ onSubmit }
/>
<div className="bulk-domain-transfer__cta-container">
<Button className="bulk-domain-transfer__cta" onClick={ onSubmit }>
<Button variant="primary" className="bulk-domain-transfer__cta" onClick={ onSubmit }>
{ __( 'Get Started' ) }
</Button>
</div>
Expand Down
19 changes: 2 additions & 17 deletions ...ing/stepper/declarative-flow/internals/steps-repository/domain-transfer-intro/styles.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,26 +23,11 @@ $heading-font-family: "SF Pro Display", $sans;
margin-bottom: 32px;

.bulk-domain-transfer__cta {
justify-content: center;
color: var(--black-white-white, #fff);
height: 48px;
/* stylelint-disable-next-line declaration-property-unit-allowed-list */
border-radius: 0.25rem;
background: var(--blue-blue-50, #0675c4);
font-size: 0.875rem;
width: 240px;
font-weight: 500;

&:hover {
background: var(--studio-blue-60);
}

&:disabled {
/* stylelint-disable-next-line declaration-property-unit-allowed-list */
border-radius: 0.25rem;
background: var(--gray-gray-5, #dcdcde);
opacity: 1;
}
justify-content: center;
border-radius: 4px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the border-radius change be done at the component level? Using button components in calypso shouldn't involve usage level CSS.

Maybe we need a new isLarge style option for the button component? height, font-weight, and border-radius could be done there. There is an isSmall prop already available.


@media (max-width: $break-medium ) {
width: 100%;
Expand Down