-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Block Support: Add border radius support #25791
Block Support: Add border radius support #25791
Conversation
ab98b18
to
c0b6ac0
Compare
This will be fantastic for gallery block 🎉 |
This tests well for me. Although it does make for a large PR by including the Search block changes I think this makes it easier for testing to have an implementation included. |
Agreed. The PR did grow larger than expected. I can split this into two to make it easier to review if you'd like. |
I find it easier to review with the search block implementation included. |
This comment has been minimized.
This comment has been minimized.
Seems like something that really ought to be implemented. For one thing, this is how the border-radius on buttons should be made available since some folks definitely want to be able to disable that #19796. Also, here's a few feature requests that would be facilitated by the block supports work in this PR: #26556 #26559. |
ae08944
to
092af28
Compare
Since this PR was created, block support has evolved quite a bit. I've rebased this and updated it to work with the latest approach to providing block support. It would be great if someone can give this another review. |
For a little background here, the border settings panel was added to contain more than just the border radius in the future. The idea being we might allow for border width, style and so on as well. The block support feature was also structured with this future in mind.
I've updated this to use sentence case for the border radius panel title, its field label and the "Display settings" panel for consistency. Unfortunately, I do not see an easy solution at the moment for standardising the order or position of the panels in the inspector controls. Besides different blocks requiring display of differing settings panels, the addition of these panels is segmented. Presently, any panels being injected via hooks appear before those added ad-hoc per block. Block styles are slightly different but they will be added before any other panels, if the block defines them. I don't think this should block this PR but we can certainly put a hold on future additions until we have a resolution on how to proceed in a consistent, scalable manner. |
+1 on considering the sidebar as secondary. We've seen in almost all user tests on WordPress.com that users have a hard time finding anything for a block in the sidebar. They go scanning the block toolbar looking for anything related to block settings. Making the block toolbar the primary interface and only falling back to the sidebar for lesser used or power settings makes the most sense to me. /cc @paaljoachim |
@aristath I've created a PR (#27220) to add a "rounded" block style for the search block. Does this fit with what you envisioned? If the block style approach is acceptable, I can remove the search block related changes from this PR. The question then is should we still add border radius block support? Others have expressed some interest in this and it does provide an initial base to which border width support etc can also be added. Ultimately, do we proceed with this PR or abandon it? |
(If I understood this correctly...) Border radius control feels more like a standard control that can be added to various blocks where the user can choose how much by slider or number to curve the corners of a block. I feel that styles is more of a unique layout option not easily accomplished by separate controls. Here is an example from the Buttons block using Twenty Twenty theme: It uses the Border Radius slider control. The Search block could also add the slider control. |
c8d807b
to
1d91e59
Compare
I think we should completely separate adding border radius block support to Gutenberg, and how it is used on the search block. Let's merge the block support here and remove search block changes. From my perspective the border support is very important to allow designers to produce different kinds of patterns using different blocks (examples here). I would include the search block in that as well, as more patterns are created for site headers for example. |
Border block support has been added following the examples set by the Typography tools and Colors support. This is intended to be extended later to include border width as an option as well.
225b0d6
to
9ab1671
Compare
I've rebased this again and removed the changes to the search block. The PR description has been updated to reflect the current state of the PR along with new testing instructions. |
There is now also a new PR containing the changes to the search block for it to opt into the border radius support and adjust inner radii etc. aaronrobertshaw#1 Once this PR is accepted and merged I'll update the new PR accordingly. |
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.
@aaronrobertshaw: nice work, thanks for moving the Search Block work out of this PR and keeping it to the point. I'm tempted to 👍 this, but would like a second opinion before merging.
const configs = [ useIsBorderRadiusDisabled( props ) ]; | ||
return configs.filter( Boolean ).length === configs.length; |
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.
const configs = [ useIsBorderRadiusDisabled( props ) ]; | |
return configs.filter( Boolean ).length === configs.length; | |
return [ useIsBorderRadiusDisabled( props ) ].every( Boolean ); |
|
||
// Further border properties to be added in future iterations. | ||
// e.g. support && ( support.radius || support.width || support.style ) | ||
return true === support || ( support && support.radius ); |
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.
return true === support || ( support && support.radius ); | |
return true === support || support?.radius; |
If you care about the return type being boolean
:
return true === support || ( support && support.radius ); | |
return !! ( true === support || support?.radius ); |
*/ | ||
export function hasBorderRadiusSupport( blockType ) { | ||
const support = getBlockSupport( blockType, BORDER_SUPPORT_KEY ); | ||
return true === support || ( support && !! support.radius ); |
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.
return true === support || ( support && !! support.radius ); | |
return !! ( true === support || support?.radius ); |
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 great @aaronrobertshaw, thanks for separating out the search block implementation.
I've added the "Needs Dev Note" in this issue so we don't forget to create a Dev Note for WP 5.7 for this kind of change: list all the new style properties that were added to blocks and users can update. This should be useful for themes and plugins to check whether they need to adapt anything. Hat tip to Riad for the suggestion. |
@@ -50,6 +50,7 @@ class WP_Theme_JSON { | |||
'--wp--style--color--link', | |||
'background', | |||
'backgroundColor', | |||
'border', |
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 values on this array need to be the name of the property as they appear in the PROPERTIES_METADATA
, so this should have been borderRadius
. There's a couple of other things I'd like to share:
- these styles are attached to the
:root
element, so I'm not sure borderRadius makes a lot of sense here - the global styles sidebar doesn't have a corresponding UI control for the user to tweak this property ― neither for the global or the group block.
In #28320 I'm removing the support for borderRadius
for the root element but could also just rename the property to fix the issue.
@aaronrobertshaw would you be available to create a follow-up PR that adds the radius control to the global styles sidebar if the block supports it?
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.
@nosolosw thank you for the explanation. I agree, a border-radius
value on :root
doesn't make a lot of sense.
It was suggested via feedback on other border radius related PRs that there should be a "global" border radius value that themes could use or supply to achieve a more consistent visual result. I mention this as having the border radius as a CSS variable on the :root
selector would help achieve this.
In #27991 I've used a similar approach adding a CSS variable for the border radius so that inner elements could adjust the value to keep uniform spacing between their border and their parent's.
It would be great to get your thoughts on that PR if you can spare the time.
@aaronrobertshaw would you be available to create a follow-up PR that adds the radius control to the global styles sidebar if the block supports it?
I can do however I had been advised to avoid adding any further sidebar controls until #27331 lands. Complicating matters and the UI, there are also requests to include support for other border properties such as width and style. I realise these aren't in place at the moment, just mentioning as they seemed to have contributed to the decision to hold off adding extra controls.
Again, I'm happy to do the follow-up if you can confirm I'm ok to do so.
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've left my comments on #27991 and #27664 (comment)
I can do however I had been advised to avoid adding any further sidebar controls until #27331 lands. Complicating matters and the UI, there are also requests to include support for other border properties such as width and style. I realise these aren't in place at the moment, just mentioning as they seemed to have contributed to the decision to hold off adding extra controls.
Can you fill me up a bit on this?
Would it work if we add the UI control for the GS sidebar but we keep the customRadius
to false? That way we have the support built-in, themes can use it via theme.json
but users won't see the controls. Same for the other border properties.
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 have a PR up adding the border radius controls to the global styles sidebar #28346
Can you fill me up a bit on this?
Some extra background on past discussions regarding controls for border related support can be found in #21540 (comment).
I'm happy to change approach. Whatever we need to get the best result.
As per #28539 (comment) I've removed the "needs dev note" label from this note. Sorry about the confusion! |
Description
Background
To allow designers to produce different kinds of patterns using different blocks there is a need to provide control over a block's border radius. Some example use cases are:
Initially, this was attempted via an adhoc approach adding the feature directly to the search block #25553. This PR adds border radius to the block support tools so that it may be leveraged by other blocks.
The intention is to also expand this to include border width and style options. There is also the possibility to allow selection of which borders show i.e. all, top, right, bottom, left, none.
Changes
How has this been tested?
Tests:
packages/block-editor/src/hooks/tests/style.js
Manually, by opting in to border radius support for test blocks such as the group and image blocks.
Testing Setup
To be able to manually test this you will need a block that has opted into the border radius support. The group block is probably easiest to do this with and will be used in the testing instructions that follow.
To opt into border radius support for the group block simply add into the
supports
setting inpackages/block-library/src/group/block.json
the following code:Testing Instructions
Screenshots
Types of changes
New feature.
Next Step
docs/designers-developers/developers/block-api/block-supports.md
once stable.Future Iterations:
Checklist: