Button Block: Use hook based border support #30194
Conversation
|
Size Change: +1.55 kB (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
|
Glad to see this! It should please lots of folks who want to be able to disable the border-radius control #19796. |
|
This might be getting close to working okay on the Do you have any pointers for me, @nosolosw? 🙏 |
|
I'll try an ad-hoc solution for now -- directly importing I hope to come up with a more scalable fix tomorrow. IIUC, we currently perform these translations implicitly for serialized attributes. Maybe we 'just' need to carry them over to the |
This might be tricky to implement 🤔 The previous block-supports mechanism strongly relied on |
|
On the edit side, maybe the |
There was a problem hiding this comment.
Looking good so far ✨
Verify that the Buttons block loads as before, and that the border radius in the sidebar panel is the one you assigned earlier.
I appreciate the PR is still a draft and a deprecation is likely on its way. Just thought I'd flag it again as we'll need to migrate the borderRadius attribute to style.border.radius.
Other than that, it is all testing as advertised 👍
Thanks! Yeah, I forgot to add a TODO item to the PR desc about the deprecation. I've added one now but still need to add (and update) snapshot tests. |
ockham
force-pushed
the
update/button-block-use-hook-based-border-support
branch
from
345c310 to
a776a91
Compare
ockham
requested review from
ajitbohra,
ellatrix,
nerrad and
ntwb
as code owners
There was a problem hiding this comment.
This looks good.
I wonder if we should merge it and iterate on the CSS variables first or if we should hold on a bit until we decide on the next steps there because it has the potential to create "a new deprecation" since it will rely on a CSS variable instead of a regular inline style.
Yeah, was wondering the same 🤔 I guess we can hold off a little longer 👍 |
ockham
added
the
[Status] Blocked
I'm probably missing some details on what's intended here, so feel free to ignore the following comments. For what it's worth though, certain environments with stricter KSES strip out CSS variables from inline styles saved to post content, so could provide gotchas with block validation when used out in the wild. I recently ran into a similar issue leveraging block support and passing colors via CSS variables to inner social link items. |
|
@aaroncampbell Thanks for raising that, I'm aware of this limitation. If we do embrace CSS variables as styles, they need to be generated server side. That said, I've been thinking of the CSS variables proposal and I'm leaning towards avoiding it at the moment (I'll comment there) |
For reference, that's this comment: #29714 (comment) Seems like we can go ahead with this PR then 🙂 cc/ @youknowriad @nosolosw |
|
The approach here sounds good to me 👍 Do we need to keep the I can do more testing (deprecations, etc) when the PR is rebased. |
ockham
force-pushed
the
update/button-block-use-hook-based-border-support
branch
2 times, most recently
from
8f525f8 to
afabceb
Compare
🥳
I could've sworn I'd seen some documentation on
Thanks! I've rebased 👍 |
|
I guess based on those findings, it's probably fine to remove However -- in the spirit of smaller iterations -- how about tackling that in a separate PR? It's arguably a rather separate concern, so I think there's some value in implementing both changes in different PRs. |
|
I decided to transform |
|
This PR will change the availability of the border radius setting and attribute for the Button block:
Are we okay with this? 😬 @nosolosw @youknowriad |
|
We should make it available for all cases. In general, we should be making all these available by default with explicit opt-out. |
@nosolosw Can we set |
|
With the current theme.json shape, to set it to true only for the button, we'd write in the {
"settings": {
"core/button": {
"border": {
"customRadius": true
}
}
}
}However, if we want to default it to true for everything it is (change the boolean): {
"settings": {
"defaults": {
"border": {
"customRadius": true
}
}
}
} |
Thanks! That's exactly what I was looking for! 😄
Let's start with just the Button block, which will retain the user-facing behavior 🙂 |
ockham
force-pushed
the
update/button-block-use-hook-based-border-support
branch
from
899ef3e to
e836de3
Compare
Done in 1e9496f. Updated PR description to reflect that the border-radius setting for the Button block is always on now. Should be ready for review now 🤞 |
|
For anyone trying to find out, how to get this work together with other {
"version": 1,
"settings": {
"color": {
"palette": [
{
"slug": "white",
"color": "#ffffff",
"name": "White"
},
...
],
...
},
"blocks": {
"core/button": {
"border": {
"customRadius": false
}
}
}
}
}At least this is what worked for me in Gutenberg v10.6.2. |
|
Hi @BenceSzalai 👋 The shape of the theme.json schema changed as part of #30541, you can find more details and the rationale for the change in #29891. The docs were also updated although perhaps the PR updating them might help shine a light on the differences. |
Description
Part of #28913.
Remove the custom border radius panel from the Button block's
edit(), and opt that button into using__experimentalBorderblock supports for border radius instead. Furthermore, enable border radius support for the Button block inexperimental-default-theme.jsonin order to preserve the current user-facing behavior.How has this been tested?
trunkbranch, create a new post, and insert a Buttons Block.Screenshots