Allow longhand and shorthand properties in theme.json and block attributes
#31641
Conversation
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
requested review from
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: +147 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
A few wee early and very initial thoughts: the border radius panel should probably start collapsed and default to PX rather than EM. I'd also put color below both border style, width and radius. We also really need the collapsed color swatch from #27331 and #27473 (comment) soon, it simply takes up too much space in this vein. Honestly I wish we could collapse the border control as far down as this:
— that is, slightly Figma inspired, a border is something you add:
We'd need for the border controls to be in a separate panel that could look like #31337, right @jameskoster? |
|
Thanks for the early feedback @jasmussen! It's very much appreciated.
The screenshot in the PR description was mistakenly captured after I'd already set border radii on the block while testing. That's why the border radius field defaulted to open, I'd set specific per-corner values for it. If you were testing via #31585 this is what I see:
The current order (in #31585) matches the order of controls in #31337. I believe the idea there was for them to match the CSS syntax order. Can you confirm I should change the order so the color controls appear last?
I can appreciate the desire for the collapsed color swatch. I was also looking at being able to add separate background & text color controls for table header, footer and striped rows as requested in #30791 (comment). The current control is definitely too unwieldy to provide that many options.
Looking at the initial designs for the collapsed color control, it looks to me like it will take quite some time to be able to implement along with required subcomponents. I don't think it should hold up the other refinements to the border block support UI ( #31337 ) as there are several PRs blocked waiting to be able to leverage this support. In addition, the controls are disabled by default which might provide the window to iterate on the color control. I'm happy to make a start on implementing the collapsed color control as soon as I free up a little extra bandwidth.
I might be missing something but this is what #31585 is looking to achieve. However, to achieve some of the controls in that UI design we needed to refine the border block support to handle non-pixel units and the split border radius values in this PR. |
The missing piece there is being able to toggle the panels themselves in the UI. I don't think we ever got to designs for that in #27331. @aaronrobertshaw This is looking good from the screenshots, but I'm not seeing the controls in my local environment. Is there anything I need to do besides building this branch? One other question: were the icons for the border style added to the icons package? |
|
Thanks for testing @jameskoster 👍
This PR itself only updates the block support etc to be capable of handling the individual border radius values. It includes updates to automated tests that can be run.
I have a separate PR ( #31585 ) based upon this one that introduces the new border radius control and refines the border block support UI. It's currently only a draft and marked as WIP as I need to confirm the approach taken both in this PR and for custom units for border width and radius. It would be great if you still had time to give this all a test. To do that, build #31585 and then enable custom borders in your
Not at this stage, they were packaged with the new border radius control. I was suffering from some tunnel vision while putting together the PR refining the border support UI. I can add them to the icons package as part of #31585 or a separate PR, whichever is best. Also, from the early discussion on that border radius control, it sounded as though it would need its own icon to represent corners rather than sides for the linked button. However, if the simple link/unlink icons used at the moment are ok, it might be possible to iterate on that control and make it more generic. Something we can work on via a separate PR. |
Asking for a friend... how does one do this? 😆 |
Sorry @jameskoster I hadn't gotten to detailing test instructions on the WIP PR. The easiest method for testing is to just toggle on the custom border properties under the |
|
I must be doing something wrong 🙈
I've done this and I'm not seeing border controls anywhere, even on the group block. |
In this case, it's most likely poor instructions 🙂 I've retested and after checking out #31585 and toggling all the border properties below to "border": {
"customColor": true,
"customRadius": true,
"customStyle": true,
"customWidth": true
},I think the problem could be that I linked to a gist that had borders enabled but was more recent. The theme.json schema has been recently reshaped. It caught me out on a couple of PRs. If you copied and pasted the entire gist it wouldn't have matched the schema and been misinterpreted. Here is a gist you can copy and paste into your theme's
The icons have been added to the icons package via a commit in #31585. |
|
No luck :( FWIW I'm using tt1-blocks which seems to have the correct declaration: https://github.com/WordPress/theme-experiments/blob/master/tt1-blocks/theme.json#L188-L193 |
|
I'm also having trouble testing this manually. I applied this patch on top of #31483 and set the following in Should I tried a few themes and seeing these controls
Are there updated instructions? |
@jameskoster I think we have to test these changes manually by checking out this PR branch #31585
Unit tests passed locally for me 👍 |
Thanks @ramonjd for testing. I'd tried to separate the changes related to updating the border radius support from the updates to the border support UI. The idea being to keep the PRs a manageable size. That's why I only had instructions related to the automated tests on this PR. Perhaps adding the gif illustrating what this PR allows caused some confusion. My apologies.
I'll update the test instructions for those that would like to see the updated block support in action. To test this support manually, we need the updated border support UI in #31585.
cc/ @jameskoster in case this helps. |
Thanks @aaronrobertshaw It's totally obvious now you point it out. :D I think I was fixated on the GIF and design feedback and assumed... |
I appreciate the feedback all the same. I'll try and make the distinction clearer on future PRs. The little comment above the gif "Via use of these changes in #31585" is pretty easy to scan straight passed and isn't exactly clear either. Thanks again for testing 👍 I've also re-tested using the new instructions and it is still working for me. |
aaronrobertshaw
force-pushed
the
update/border-radius-support-custom-corners
branch
from
677a9a8
to
6408a75
Compare
aaronrobertshaw
changed the title
| $corner = strtolower( preg_replace( '/(?<!^)[A-Z]/', '-$0', $key ) ); | ||
| $styles[] = sprintf( 'border-%s-radius: %s;', $corner, $radius ); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Can the border radius be null? Do we need to check for that like we do for spacing?
There was a problem hiding this comment.
I believe the original code here for the border support was added prior to _wp_array_get getting wider usage so the code approaches the check a little differently to the spacing support.
Along with checking for the feature support, it also does an isset check which would return false for a null value. Would making the supports' code consistent in approach be best done in a separate PR? I'm happy to take care of that in a follow up.
|
There seems to be some conflict in files and I've left some minor comments. I think once that's addressed we can merge this one (I can manually test then, haven't done yet). |
This updates the theme.json generated stylesheet to handle individual border radius values per corner. It maintains single string value support for the shorthand expression for all corners at once. This allows backwards compatibility for blocks that have already adopted border radius support.
Maintains support for flat string value for setting all border radii at once.
aaronrobertshaw
force-pushed
the
update/border-radius-support-custom-corners
branch
from
706321b
to
9b8f2c7
Compare
The conflicts turned out to be only for the assertions relating to generated stylesheets. I've resolved those and the tests are all green again. I'll rebase the PR updating the border support UI onto this shortly so it can be used for manual testing if desired. |
There was a problem hiding this comment.
Tested this using padding -both short and longhands- in the front, site, and post editor. Also tested that content created in trunk works well with this PR (no invalidations, etc).
Thanks to your patience through this we're getting a great simplification in the implementation + new behaviour across the board. 🌟
I'm just crossing my fingers for this not to cause too many headaches when/if we need to port late things to core for 5.8 😅 🤞 🍀 I'll take care of that should it happen.
oandregal
changed the title
theme.json and block attributes
| $attrs[] = 'border-top-left-radius'; | ||
| $attrs[] = 'border-top-right-radius'; | ||
| $attrs[] = 'border-bottom-right-radius'; | ||
| $attrs[] = 'border-bottom-left-radius'; |
There was a problem hiding this comment.
Was this change backported to Core at any point?
There was a problem hiding this comment.
It appears to me that the changes were backported and were done so in (WordPress/wordpress-develop@f034bc8).
There was a problem hiding this comment.
Part of my confusion is that the function indicates that it should be removed once the minimum WP version is 5.8, I guess it should have been updated to 5.9 after this change.
There was a problem hiding this comment.
That was an oversight of mine sorry. This change didn't make it into a release until 11.0 and therefore WP 5.9.
An additional attribute was added to this filter a few months later (#36280) that would also make the minimum version requirement 5.9 before it can be removed.
I've created a tiny PR to fix this comment in #38359.
Related: #31337, #31585
Depends on: #31483Description
This PR:
margin: 10pxormargin-left: 20pxThe driving force behind this was achieving the refined border support UI in #31337. To do so, the border block support needs to allow custom values for each corner's border radius. This PR addresses this need while also maintaining backwards compatibility for a single border radius value.
How has this been tested?
Automated tests.
Theme.json stylesheet generation:
/var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-theme-json-test.phpBlock support inline styles:
packages/block-editor/src/hooks/test/style.jspackages/edit-site/src/components/editor/test/global-styles-renderer.jsManual testing:
core/groupblock in the theme.json as well. Try simple shorthand margin and padding valuesBy block typetab, then expand the Group block panel.This PR does not update the border support UI, to test the border changes manually, try #31585
update/border-block-support-ui(that's all that is needed, it contains the changes from this PR)
npm install)Screenshots
Via use of these changes in #31585
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).