-
Notifications
You must be signed in to change notification settings - Fork 4.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
Button: Update disabled state to be without background. #50496
Conversation
Size Change: +4.75 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
Flaky tests detected in 63d9394. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4960497871
|
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.
Looks good to me 👍 I just pushed some minor cleanup.
@@ -118,21 +118,15 @@ | |||
outline: 1px solid transparent; | |||
|
|||
&:active:not(:disabled) { | |||
background: $components-color-gray-300; | |||
color: $components-color-accent-darker-10; |
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.
Not that I object but just checking since it wasn't mentioned in the PR description — is it intentional for the text to turn black when clicked?
CleanShot.2023-05-13.at.00.20.06.mp4
&:disabled, | ||
&[aria-disabled="true"], | ||
&[aria-disabled="true"]:hover { | ||
// TODO: Prepare for theming (https://github.com/WordPress/gutenberg/pull/45466/files#r1030872724) |
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.
I think that this comment still applies, right? We should still think about adding variables at the theme level for disabled UI
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.
The TODO comments were specifically for the issue of darken
/lighten
Sass functions being used on theme variables.
As for the theme-ready gray variables, we simply haven't put them to use anywhere yet. So I was kind of thinking of that as a big separate task rather than a contextual TODO item 😄
Thanks for all the help! I'm going to merge this one but always happy to follow up. |
What?
The disabled state of notably the tertiary variant of the button component adds a background:
This is incorrect for that particular variant, this PR removes it:
Incidentally that unifies its style with this "Saved" disabled button as well.
The PR also updates some incorrect hover and active states.
Why?
The primary button is filled, the secondary button is outlined, the tertiary button is without background. This makes that consistent for the disabled state.
Testing Instructions
Create a new post, and before adding any content, observe the save draft and preview buttons.