Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Block Support: Add border radius support #25791
Conversation
aaronrobertshaw
changed the title
aaronrobertshaw
requested review from
ajitbohra,
ellatrix,
Soean,
spacedmonkey,
talldan,
TimothyBJacobs and
youknowriad
as
code owners
|
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 hidden.
This comment has been hidden.
apeatling
requested review from
nosolosw and
youknowriad
and removed request for
youknowriad
lib/experimental-default-theme.json
Outdated
Show resolved
Hide resolved
packages/block-library/src/search/block.json
Outdated
Show resolved
Hide resolved
|
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. |
aaronrobertshaw
force-pushed the
aaronrobertshaw:try/border-radius-block-support
branch
|
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. |
| $border_radius['wrapper_style'] = sprintf( 'border-radius: %spx;', esc_attr( $radius_value ) ); | ||
|
|
||
| return $border_radius; | ||
| } |
youknowriad
Nov 2, 2020
Contributor
I was hoping we'd avoid the need for any particular code on blocks when you enable "support" for this? why is this needed?
I was hoping we'd avoid the need for any particular code on blocks when you enable "support" for this? why is this needed?
aaronrobertshaw
Nov 2, 2020
Author
Contributor
This was required to maintain visual consistency between the border radius for the block itself and its inner elements when appropriate.
When the search block is configured to position the button "inside", the border radius that the search block opted into needs to be applied not only to the wrapper around the search input and button but also to the input and button inner elements. This is complicated a little further as they need to be adjusted for the inner padding of the wrapper so that the border radius can match visually with the wrapper's border radius and we do not get "fat" corners.
This addresses an issue highlighted with the original PR that sparked this one.
This was required to maintain visual consistency between the border radius for the block itself and its inner elements when appropriate.
When the search block is configured to position the button "inside", the border radius that the search block opted into needs to be applied not only to the wrapper around the search input and button but also to the input and button inner elements. This is complicated a little further as they need to be adjusted for the inner padding of the wrapper so that the border radius can match visually with the wrapper's border radius and we do not get "fat" corners.
This addresses an issue highlighted with the original PR that sparked this one.
youknowriad
Nov 3, 2020
Contributor
Do you think it's possible to leverage some CSS magic to fix that instead? I'm asking cause this makes me wonder whether the block support is the right approach for this particular block or not. Basically, why is it even a block support if it requires such custom code, it removes the declarativeness of the block support.
Block supports are and should be deterministic IMO: you apply them, you get a block radius automatically applied without custom tweaks. So it makes wonder whether block radius support flag is better suited for simpler blocks say button for instance or a group...
Do you think it's possible to leverage some CSS magic to fix that instead? I'm asking cause this makes me wonder whether the block support is the right approach for this particular block or not. Basically, why is it even a block support if it requires such custom code, it removes the declarativeness of the block support.
Block supports are and should be deterministic IMO: you apply them, you get a block radius automatically applied without custom tweaks. So it makes wonder whether block radius support flag is better suited for simpler blocks say button for instance or a group...
aaronrobertshaw
Nov 3, 2020
Author
Contributor
Do you think it's possible to leverage some CSS magic to fix that instead?
That's a good question, I'll look into that. Not being able to use an inherited value within calc() might make it tricky. Maybe CSS variables could offer something.
One other thing that could complicate this, arose from some internal discussions, which raised the possibility of adding spacing/padding and border width support, both of which would affect the calculations for the inner border radii.
I'm asking cause this makes me wonder whether the block support is the right approach for this particular block or not. Basically, why is it even a block support if it requires such custom code, it removes the declarativeness of the block support.
That's true regarding the search block itself. However, as you mention, simpler blocks could benefit from the border radius support and keep that declarative nature. If the search block still needs to offer a border radius feature, would it not be better to reduce duplication of code and leverage the block support for border radius?
We still have the initial PRs trialling adding the border radius feature for the search block in an ad-hoc manner, #25553 & #25203.
If you think that approach is actually the better option for the search block, I'm happy to remove the modifications to that block from this PR. It would be a shame though not to proceed with adding border radius support that other blocks can start to use.
Do you think it's possible to leverage some CSS magic to fix that instead?
That's a good question, I'll look into that. Not being able to use an inherited value within calc() might make it tricky. Maybe CSS variables could offer something.
One other thing that could complicate this, arose from some internal discussions, which raised the possibility of adding spacing/padding and border width support, both of which would affect the calculations for the inner border radii.
I'm asking cause this makes me wonder whether the block support is the right approach for this particular block or not. Basically, why is it even a block support if it requires such custom code, it removes the declarativeness of the block support.
That's true regarding the search block itself. However, as you mention, simpler blocks could benefit from the border radius support and keep that declarative nature. If the search block still needs to offer a border radius feature, would it not be better to reduce duplication of code and leverage the block support for border radius?
We still have the initial PRs trialling adding the border radius feature for the search block in an ad-hoc manner, #25553 & #25203.
If you think that approach is actually the better option for the search block, I'm happy to remove the modifications to that block from this PR. It would be a shame though not to proceed with adding border radius support that other blocks can start to use.
aristath
Nov 13, 2020
Member
I honestly don't see any benefit to adding a border-radius control to the search block...
Border-radius is something that should be consistent on all elements on a page. Having different border-radii in different elements on a page creates a visual discontinuity and is a big no-no design-wise. It's OK to have some elements rounded while others are square(ish), but having a border-radius of 7px on some elements while other elements on the same page have 3px is not something that should be done.
With that in mind, I'd say that themes should add a global border-radius in their theme.json file and use it consistently across the board for all their blocks - directly in their CSS files. The search block can have a separate "rounded" style, but there's no need for a border-radius control... It can simply be a block-style.
I honestly don't see any benefit to adding a border-radius control to the search block...
Border-radius is something that should be consistent on all elements on a page. Having different border-radii in different elements on a page creates a visual discontinuity and is a big no-no design-wise. It's OK to have some elements rounded while others are square(ish), but having a border-radius of 7px on some elements while other elements on the same page have 3px is not something that should be done.
With that in mind, I'd say that themes should add a global border-radius in their theme.json file and use it consistently across the board for all their blocks - directly in their CSS files. The search block can have a separate "rounded" style, but there's no need for a border-radius control... It can simply be a block-style.
aaronrobertshaw
Nov 13, 2020
Author
Contributor
I agree with the need for consistency of border radius across different elements on the page.
One point worth noting here though is that differing border-radius can actually be needed to maintain visual consistency. For example, so that nested border radii actually appear consistent with each other rather than having "fat corners".
An image highlighting this can be found in this comment.
I agree with the need for consistency of border radius across different elements on the page.
One point worth noting here though is that differing border-radius can actually be needed to maintain visual consistency. For example, so that nested border radii actually appear consistent with each other rather than having "fat corners".
An image highlighting this can be found in this comment.
aristath
Nov 13, 2020
Member
Yeah, but that is for elements in the same block.... So it's something that can be done directly in the theme's stylesheet, and values can be calculated easily via CSS calc. However, I was referring to cross-block inconsistencies. We can't have galleries use a 10px border-radius, search using a 6px radius, 1 group using 20px and search using something different... There needs to be consistency. And that consistency can be accomplished with simple CSS styles on the theme itself - without the need for any extra controls.
Adding a control to change the border-radius of a specific element will ultimately lead to visual inconsistencies since it won't follow the overall tone set by the theme. The search-block should either follow the global theme's border-radius (taking into account nested border-radii where needed in the theme's styles), or have a rounded style. Anything other than that would cause visual inconsistencies. So I really don't see why this can't be a simple default/rounded block-style...
Yeah, but that is for elements in the same block.... So it's something that can be done directly in the theme's stylesheet, and values can be calculated easily via CSS calc. However, I was referring to cross-block inconsistencies. We can't have galleries use a 10px border-radius, search using a 6px radius, 1 group using 20px and search using something different... There needs to be consistency. And that consistency can be accomplished with simple CSS styles on the theme itself - without the need for any extra controls.
Adding a control to change the border-radius of a specific element will ultimately lead to visual inconsistencies since it won't follow the overall tone set by the theme. The search-block should either follow the global theme's border-radius (taking into account nested border-radii where needed in the theme's styles), or have a rounded style. Anything other than that would cause visual inconsistencies. So I really don't see why this can't be a simple default/rounded block-style...
aaronrobertshaw
Nov 13, 2020
Author
Contributor
However, I was referring to cross-block inconsistencies.
Sorry, that is what I meant by I agree with the need for consistency across different elements on the page. I was only trying to highlight that not all border radius values would be the same everywhere.
Adding a control to change the border-radius of a specific element will ultimately lead to visual inconsistencies since it won't follow the overall tone set by the theme.
It could well do, but to play devil's advocate, should the end user have the freedom? We allow the option for users to choose random colors potentially destroying the look and feel of a theme.
The search-block should either follow the global theme's border-radius (taking into account nested border-radii where needed in the theme's styles), or have a rounded style. Anything other than that would cause visual inconsistencies. So I really don't see why this can't be a simple default/rounded block-style...
This arose while attempting to build new block patterns that included border radii within the design for the pattern. I'll have to confirm the pattern previews can actually inherit from the global styles. If we are essentially tying the end user's hands with regards to the border radius values once the pattern is inserted, we would need to limit any disjoint between what they see in the preview vs the end result. That should be the case anyway but would be of lesser consequence if they have full control.
However, I was referring to cross-block inconsistencies.
Sorry, that is what I meant by I agree with the need for consistency across different elements on the page. I was only trying to highlight that not all border radius values would be the same everywhere.
Adding a control to change the border-radius of a specific element will ultimately lead to visual inconsistencies since it won't follow the overall tone set by the theme.
It could well do, but to play devil's advocate, should the end user have the freedom? We allow the option for users to choose random colors potentially destroying the look and feel of a theme.
The search-block should either follow the global theme's border-radius (taking into account nested border-radii where needed in the theme's styles), or have a rounded style. Anything other than that would cause visual inconsistencies. So I really don't see why this can't be a simple default/rounded block-style...
This arose while attempting to build new block patterns that included border radii within the design for the pattern. I'll have to confirm the pattern previews can actually inherit from the global styles. If we are essentially tying the end user's hands with regards to the border radius values once the pattern is inserted, we would need to limit any disjoint between what they see in the preview vs the end result. That should be the case anyway but would be of lesser consequence if they have full control.
aristath
Nov 13, 2020
Member
but to play devil's advocate, should the end user have the freedom?
We're about to enter a more philosophical discussion if we go down that path, but there is such a thing as too much control and a difference between control and enabling poor design choices.
This arose while attempting to build new block patterns that included border radii within the design for the pattern.
Block-patterns can use block-styles. And a block-style simply adds a CSS-class to the block. So having a is-style-round class added to the search block is all that's really needed - which can be accomplished with a plain block-style selector like buttons have, and simple CSS 😉
but to play devil's advocate, should the end user have the freedom?
We're about to enter a more philosophical discussion if we go down that path, but there is such a thing as too much control and a difference between control and enabling poor design choices.
This arose while attempting to build new block patterns that included border radii within the design for the pattern.
Block-patterns can use block-styles. And a block-style simply adds a CSS-class to the block. So having a is-style-round class added to the search block is all that's really needed - which can be accomplished with a plain block-style selector like buttons have, and simple CSS
|
|
||
| // Minimum inner radius of 1px as having none when there is an outer radius jars. | ||
| const MIN_INNER_RADIUS = 1; | ||
|
|
||
| export default function SearchEdit( { |
youknowriad
Nov 2, 2020
Contributor
Same here, it feels there are a lot of changes here where I'd have expected everything to be absorbed by the support flag and hooks?
Same here, it feels there are a lot of changes here where I'd have expected everything to be absorbed by the support flag and hooks?
|
Hey Aaron Nice job! This is what I see: In the version I see. The sidebar settings do not contain Border Settings. There are two toggles in the toolbar, and one drop down. The user will need to click the icons in the toolbar to try them out. Brainstorming... For an easier overview the toggles can be added to the sidebar panel. Containing easy to see labels. Here is one suggestion: A question shows up. Button drop down options: Are these actually styles? Looking at the Social Icons block. It has Styles for Default, Logos Only and Pill Shape. Edit: The above are explorations. Thinking of different ways the options can be presented. Jon and Joen I am thinking you might have some comments on this PR as well. |
|
I'll share some more thoughts next week, I think there's a chance to systematize this, as I'd agree generally that the addition feels a little random, even if useful. We need to think in terms of how this can be applied to all blocks, and form a coherent system.
I would not do this, though. The block itself is the primary interface, and anything you put in the sidebar should be considered secondary, even if there's an argument to be made that some of those configurations could be secondary. So I'd keep the items in the toolbar as they are now, even if those options could use some systematization and cleanup separately. |
|
Hi @paaljoachim, thanks for taking the time to review this.
Checking out this PR branch I see the border settings in the sidebar. I've also re-built Gutenberg but cannot replicate the behaviour you describe. Below is a new gif of what I see.
In both my tests this morning, I've fired up the Gutenberg environment with this PR branch checked out already. It could be an issue with switching from master to this branch with Gutenberg env already up and being watched. I'll rebase this branch and test again. |
aaronrobertshaw
force-pushed the
aaronrobertshaw:try/border-radius-block-support
branch
to
eae2c87
|
After rebasing this, the border settings are still showing correctly for me. Switching between master and this branch did require a hard refresh of the editor though. |
|
Some further thoughts. Most importantly, I appreciate the work on adding additional design tools to our blocks, enabling better patterns and creations. Please do keep this appreciation for the work in mind as you read the following, because the feedback is not just about this PR, but about the approach in general. However as this accelerates, it's increasingly clear that the sidebar is not able yet to scale to accommodate these new controls:
More importantly, existing patterns even if arbitrary, aren't followed, so for example in this case "Border settings" is an existing panel for the Buttons block, where it's the 3rd panel whereas here it's the first panel. Additionally, the label case is wrong, "Border Settings" where it should be "Border settings" per past decisions. I've been working with some others on some systems for how controls can be added to blocks in a predictable and consistent way, it's hard work and it's taken a while and I expect and hope that it's shareable very soon. But in the mean time, we need to find a more holistic approach that is less prone to fragmenting the experience. That approach could start with a conversation around how the new design tool fits into global styles, and from there, how it is then surfaced on a per-block basis. |
|
@jasmussen Thank you for sharing your thoughts and tackling these issues, it is appreciated. Working through a solid process from initial conversations around the new design tool through to how it is surfaced, sounds great. If I can help at all with that, please let me know. In the meantime, I can correct the panel title to use sentence case. The ordering of the panel to match other blocks might be a little trickier as it currently gets injected automatically via block support and different blocks will have a different number of panels. I'll see what I can do there to improve that situation. It would be good to still be able to finesse this to a point it could be merged. As we build additional block patterns and discover the need for new design tooling, I'll make sure we loop in anyone we need to, into the discussion. Who would be best to include in these discussions? |
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. |
aaronrobertshaw
force-pushed the
aaronrobertshaw:try/border-radius-block-support
branch
from
c8d807b
to
1d91e59
|
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.
aaronrobertshaw
force-pushed the
aaronrobertshaw:try/border-radius-block-support
branch
from
225b0d6
to
9ab1671
aaronrobertshaw
changed the title
|
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. |
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.jsManually, 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
supportssetting inpackages/block-library/src/group/block.jsonthe following code:Testing Instructions
Screenshots
Types of changes
New feature.
Next Step
docs/designers-developers/developers/block-api/block-supports.mdonce stable.Future Iterations:
Checklist: