-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
--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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be done globally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this fix on Stepper level. It seems a couple of flows were affected by updating @wordpress/components
.
p1689919222911329-slack-C05CT832K2T
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Omar, thank you for working on this one. Looks nice!
Besides the disabled state that you mentioned, I just missed the border radius 4px;
Great catch, fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
_wp-components-overrides.scss
by increasing selector specifity
Thanks for fixing this! I noticed two issues in the link in bio flow: Screen.Recording.2023-07-21.at.1.58.02.PM.mov
|
opacity: 1; | ||
} | ||
justify-content: center; | ||
border-radius: 4px; |
There was a problem hiding this comment.
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.
@@ -11,14 +11,16 @@ | |||
} | |||
|
|||
/* @wordpress/components Button overrides */ | |||
.components-button { | |||
a.components-button, | |||
button.components-button { |
There was a problem hiding this comment.
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:
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Louis! Much appreciated.
button.components-button.bulk-domain-transfer__cta { | ||
&:disabled { | ||
background: var(--studio-gray-5); | ||
} | ||
} |
There was a problem hiding this comment.
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?
Thanks for your work here @alshakero. Based on it, I've created this follow-up task that fixed the style issue. So we can probably close this one. |
I really really appreciate it! I was deep in other tasks. |
Follow up to #79692.
This PR increases the selectors in
wp-components-overrides
in make wp.com styles take precedence over wp.org styles (that come from@wordpress/components
.This PR will fix all Stepper flows.
Testing