Block Support: Add configurable sides and margins to spacing support #30333
Conversation
- Adds margin block support - Updates padding block support to allow customisable sides - Does not update global styles sidebar yet
aaronrobertshaw
added
[Status] In Progress
|
Size Change: +515 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
- Fixes bug where BoxControl cannot be reset in Global Styles - Updates the spacing panel to include margins and custom sides
aaronrobertshaw
requested review from
oandregal,
youknowriad,
apeatling and
glendaviesnz
aaronrobertshaw
changed the title
aaronrobertshaw
requested review from
ajitbohra,
chrisvanpatten,
ellatrix and
spacedmonkey
as code owners
|
This looks like a duplicate of #30371 right? |
|
Looks like your PR proceeded mine, let's see what differences are there between the two. |
I'd guess the biggest difference would be using the modified |
|
@aaroncampbell that sounds like a useful feature for padding as well for instance, can we extract it to its own PR? |
The modified This PR built on that by adding both margins block support as well as the ability to configure which sides are supported for both padding and margins. |
|
What functionality would you like to see extracted to its own PR? Or, is it easier to proceed with this? |
since we already have a padding control using the "BoxControl", it seems a PR with the modified BoxControl (and use it for padding) would be focused enough? We can then circle back to the margin block support one (which should just work and support partial margins). WDYT |
Is the purpose of splitting this up, to create PRs that do the same thing, to create a more granular paper trail of changes? The first PR ( #29363 ) mentioned in the previous comment, already modifies the This PR uses that new ability to configure sides for the |
yes, that's exactly my reasoning; if a PR can be focused on a single thing and adds value on its own, it makes reviews and merging easier. |
|
Ok, my thinking was the modified BoxControl and using it as the control to set separator block top/bottom margin was a sufficiently small but actually meaningful change. Then this update of the spacing support was another logical unit of changes for a PR. |
|
@aaronrobertshaw Any reason why it's not the other way around, why the separator block is not using the block supports? I actually see this in three steps:
|
This decision was made due to the length of time it generally takes to get other block support related changes approved and merged. The idea was to simply tweak the BoxControl and use it, providing the needed functionality on the separator block sooner. The margin values are saved under the same attribute location they would eventually be once margin support was in fact added. Allowing for the block to simply opt-in to support and remove the explicit control, a hot swat almost, no deprecation needed. Given discussion around |
|
Fair point, we do need the block supports in a good place before merge. For that particular one I don't see any blockers personally. It seems like "auto" is not something we want there for now and that the only thing missing in my PR is the UI in global styles which I believe this PR does already, so I think we're very close. (this PR seem to be missing the change to the layout though to force centered blocks) For the visualizers, we already don't show the visualizers for Group block in trunk (basically we don't show them for any block where we can't add the component in "Edit" without creating a mismatch between frontend and backend. This is something we need to solve by changing how the visualizers work: potentially use a popover for them and shouldn't stop us from landing the other PRs, the visualizers are not mandatory for these controls to work. |
I wasn't certain about this. If left/right margins are being ignored via forcing |
Depending on the container, it might make sense to allow them. The default "vertical" layout is just one possible layout, the "columns", "social links", "navigation" are all examples of "horizontal" layouts where left/right margins make sense. That said, it's a good point and we might want to be able to hide these and potentially avoid |
Description
BoxControlfrom Separator Block: Add top and bottom margins without resizable box #29363 to allow arbitrary sides to be configuredBranched from #29363 which should be close to being able to land
How has this been tested?
Simple unit test for spacing inline styles:
npm run test-unit:watch packages/block-editor/src/hooks/test/style.jsManually.
Test Setup
customMarginandcustomPaddingundersettings.defaults.spacingto true ( gist )block.jsonfile, opt into padding support.Then configure support for only top/bottom margins ( gist ).
Test Instructions - Block Support
BoxControlicon for margins should reflect the configured sides as well.Test Instructions - Global Styles
core/groupblock context ( gist )Screenshots
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).