-
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
Update the buttons block to use the new color support flag. #21266
Conversation
Size Change: -315 B (0%) Total Size: 889 kB
ℹ️ View Unchanged
|
__experimentalLinkControl as LinkControl, | ||
} from '@wordpress/block-editor'; | ||
import { rawShortcut, displayShortcut } from '@wordpress/keycodes'; | ||
import { link } from '@wordpress/icons'; | ||
|
||
const { getComputedStyle } = window; | ||
|
||
const applyFallbackStyles = withFallbackStyles( ( node, ownProps ) => { |
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.
@jorgefilipecosta What this fallback styles for?
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.
It was used to compute the colors the button contained when no colors were explicitly assigned than the colors were used for contrast checking.
fefd666
to
96dd0ab
Compare
@jasmussen do you know why we had a "div" wrapper around button blocks? |
Are you referring to this one? I don't actually know, but I can't see why it would be necessary. |
@jasmussen yes, there's the same on frontend too. |
96dd0ab
to
de8a6da
Compare
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 Riad, awesome work here things worked well in my tests.
I left some comments the more important ones are related to inheritance. That part will be challenging.
...localAttributes.current?.style, | ||
color: { | ||
...localAttributes.current?.style?.color, | ||
gradient: value, |
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.
What is the reason for making gradient attribute child of color and not making it a "sibling" of color?
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.
because it's kind of related to the backgroundColor in terms of UI..., but I don't mind changing it. Not sure what's best.
}; | ||
}; | ||
|
||
const onChangeGradient = ( value ) => { |
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 wonder if it would make sense to include this logic inside PanelColorGradient and avoid the need for effect. E.g: include a onStyleChange that when passed makes the component update the keys in a style attribute?
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'd love a single onChange for both onColorChange
and onGradientChange
instead of two sequential calls (I don't know if that's what you're talking about?)
__experimentalLinkControl as LinkControl, | ||
} from '@wordpress/block-editor'; | ||
import { rawShortcut, displayShortcut } from '@wordpress/keycodes'; | ||
import { link } from '@wordpress/icons'; | ||
|
||
const { getComputedStyle } = window; | ||
|
||
const applyFallbackStyles = withFallbackStyles( ( node, ownProps ) => { |
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.
It was used to compute the colors the button contained when no colors were explicitly assigned than the colors were used for contrast checking.
5c7f989
to
cb39108
Compare
@nosolosw @jorgefilipecosta I updated the PR to use the inline style styles (like before) solving the inheritance issue. I also restored That said, you can see that in 2020, the palette colors don't seem to work properly on the editor due to a specificity problem (work well on the frontend). On 2019, we have the palette issue mentioned here #21428 (comment) On themes without editor styles, everything works as expected. |
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, @youknowriad thank you for the changes, the inheritance problems seem fixed.
I just noticed a small regression on the contract checking:
Tested in: Twenty Seventeen
I added a buttons block.
I selected a gradient as background e.g: from black to black (default color text is white).
I saw a contrast warning.
I guess contrast checking should not be enabled when a background gradient is selected?
Thanks for the review and good catch. I disabled the contrast checking when a gradient is selected. I also extracted the API in #21481 to land it first |
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 issue was fixed in my tests 👍
So happy to see the lighter DOM and global styles happen. This is a PR that needs to land. I just tested a recent 2019 patch which fixes many of the issues that have happened with it, so I'm on the "latest version", so to speak, and I'm seeing some issues with that theme. In master: In this branch: And this is the frontend, and therefore presumably what the button should look like: This appears to be because of some rules in the editor style that target the button directly. These obviously need to be updated:
Absent that rule, the default styles are inherited and shown:
So yeah, there's breakage. But IMO it's worth it, because it means the editor itself can inherit identical styles to the frontend. Almost certainly unrelated to this particular PR, as we continue to add customization options to blocks, the default styles definitely need revisiting. It's a little weird that the default button block starts rounded and with a border-radius set to zero: As seen in that GIF there are a number of issues:
Let me know which of these to ticket separately. |
That's unrelated, it just saves the "default" style variation for the block, next time you insert the block, it will have that variation by default. I believe there are some UX explorations to improve this on separate issues.
That's because applying the background color, applies the backgroundColor CSS property, I thought it was the same before but maybe the removal of the wrapper had an impact on its behavior, I'll have to check. |
I took a look at the outline style variation and it seems to work in a similar way on master, which is IMO consistent, you're applying a background color not a "theme color" or something like that. |
The border radius issue is also a bug already present on master, I reopened this issue to track it #17596 |
I do think it dilutes the outline version. But because it's clearly unrelated to this PR, totally good to disregard entirely :) Thanks for opening the radius ticket! |
I wonder if it's better to have a border color instead of a style variation personally, it's clearer for the user. |
Possibly yes. Though I do think an "outlined button" is something that's good to have a thing for — and a class attached to. |
I merged this, I'll try to add a note about the markup change for the next Gutenberg release. |
Started a ticket for this here: https://core.trac.wordpress.org/ticket/49863 |
I was pleasantly surprised to see that one of the containers 'button' block has been removed in this commit.
On second thought, I would keep in mind that developers may initially try to go to the button block for actions of the traditional button (submitting a form, toggling user-defined settings on a page, etc). |
Just a quick question. This change impacts a lot of our themes. See: #21747 Were there any plans to make it backwards compatible for folks already running these themes?
It would be great to have these available as soon as possible, or before these PRs are merged into master. Whatever your workflow dictates. Thanks a lot! |
This PR does a lot of things:
Notes