Button: update spacing support to use axial padding #33859
Conversation
|
Size Change: +284 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
stacimc
changed the title
stacimc
changed the title
stacimc
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
|
Pinging @jasmussen as this PR relates to your concerns here: #32610 (comment) I was discussing this a little with @andrewserong and @aaronrobertshaw, and considering potential issues with the Button block behaving differently when you unlink the sides in the padding controls compared to other blocks. One thought is that we could get rid of the linking/unlinking behavior for Button; so, only display the axial controls with no option to link/unlink. This prevents any frustration that might be caused by expanding the box control and perceiving missing options/less granularity than they may see in the padding controls for other blocks. I'm unsure if that's necessary, or if it's consistent with other use cases for axial controls. Gap, for example, seems like it would benefit from having both general and axial spacing. |
|
Also pinging @mtias since this is a follow-up on your request here: #31774 (comment) :) |
|
Thanks for working on this! Here's before:
And here's after:
On the one hand, it's nice to be able to control both sides on an axis, and is very probably what you want most of the time. On the other hand, this now becomes impossible:
I would consider it an edgecase, but it's also always important to be aware of the opportunity cost: which creative designs are we making impossible by our changes, even if well-intended? Additionally, the axial split controls are surfaced after you untoggle the "link" button, meaning you've already made a choice to surface the advanced padding controls. In this case, I tend to think that axial padding support is possibly something we can implement in a more visual way such as #33221. In that interface we might enable axial resizing when holding Alt and dragging. Taking that a step further, we might even support Alt + dragging on the input control to do the same in the 4-split input controls. Axial controls feel the most potent when affecting interfaces that are already technically locked in a similar way, such as column blocks where the axes control the gap property between them. Or in cases where we are absolutely sure we want to limit the configuration. In this case, I'm not convinced. But it's also not a strong opinion, and I'm happy to defer. Nice work! |
For context (description updated now), I added the padding support on Button very recently in #31774, at which time it wasn't possible to enable axial selections via the block support. This is meant to be a relatively fast follow-up to address a suggestion to limit the controls to axial sides until we see concrete requests for padding support on individual arbitrary sides (at which point we could easily go back). My take on this is that controlling top/bottom or left/right padding as pairs is likely what users want most of the time, and is a little cumbersome using the four individual side controls. It's possible by holding down the I do worry about making it impossible to achieve those edge cases with mismatched padding on axial sides -- maybe we get the best of both worlds by limiting the Inspector Controls to the axial sides, and allowing individual side padding via the visual controls in the future? This makes the common use case very easy via Inspector Controls but allows finer customization via the more advanced/"power feature".
+1 What do you think about removing the linking/unlinking here? So specifically for Button, we display the axial controls by default with no link/unlink button. The obvious downside is we lose the ability to change general padding for all sides very quickly (you can obviously still individually update the two inputs). This feels less cumbersome to me than updating the four individual inputs. |
That's an interesting thought! With that in mind, we might actually want to just try this PR as is, and if the use case I described comes back to haunt us, we can explore that. 👍 👍 |
|
@stacimc I haven't had a chance to test this properly yet, but nice work hooking into the axial controls! I think we might need to also update the global styles spacing panel to ensure that the Here's where the function lives: We might be able to borrow a bit of the logic from the axial-input-controls to keep it consistent? |
Good catch, thank you! I will take a look at this :) |
|
This code change is looking good to me @stacimc, and nice inclusion of the validity check. Looks like this just needs a rebase now that the ToolsPanel is in, but I’m happy to give this another look over and test once it’s ready! |
If a spacing support is configured with an invalid mix of axial and individual sides, log a warning and disable the feature. If only one feature is invalidly configured, this does not affect the other; eg if padding is configured incorrectly but margin is not, padding is disabled but margins will continue to work.
Previously the hook was only used if the feature was known to be supported. Now that it is used in checking the spacing configuration, it is possible for `support` to be undefined here.
stacimc
force-pushed
the
update/axial-padding-support-for-button
branch
from
4add752 to
fddeb7e
Compare
|
This has been updated to include the changes from the new Dimensions panel (#32392). I've also made padding a default control for Button. |
There was a problem hiding this comment.
Thanks for the work here @stacimc, this looks great to me!
✅ Axial controls are working correctly for me within the post editor
✅ Axial controls are working correctly for me within the site editor
✅ Specifying individual sides in the block.json file works as expected
✅ Specifying axial sides in the block.json file works as expected
✅ Console warning for a support including both axial and arbitrary sides makes sense to me and the UI for the support is hidden, but the existing block doesn't throw a validation
This all LGTM, so I've given it the ✅, but it'd be great to get a couple more pairs of 👀 on this, too, to see what other folks think about the approach for checking that the support is valid. I was also going to suggest using the plural of axis instead of "axial sides" but given that the word looks the same as the plural for the word "axe", I think "axial sides" is a better trade-off 😀
|
Ran through every check and this worked as designed. 👍 I think the notice in the console is standard for letting developers know when something is deprecated or not implemented as expected, so this seems like a good place to me. I think the more visual representation of padding on the canvas that @jasmussen linked to could be followed up on after this initial round of tools work is completed, as we continue to refine the tools and the UI we are adding. |
|
Thanks for this follow up! This is a much better default.
@jasmussen I'm fine with this not being possible. It can always be added later if there's a strong need for it. Right now, allowing arbitrary values on Button feels like it will invariably produce not great results. |
mtias
added
the
[Feature] Design Tools
Description
This is a follow up to #31774. This PR updates the spacing block supports to provide axial padding as an option. It also updates the Button block to use general and axial padding (as opposed to padding for each of the individual sides).
I added the padding support on Button recently in #31774, at which time it wasn't possible to enable axial selections via the block support. This PR is meant to be a relatively fast follow-up to address a suggestion to limit the controls to axial sides (anticipated to be the most common use case) until we see concrete requests for padding support on individual arbitrary sides (at which point we could easily go back).
In an example
block.json:This block would support both margin and padding for the top and bottom sides. However the margin can be controlled for the top and bottom individually, while the padding UI will only display a 'vertical' control to update both sides at once.
There are a couple of things I'd like to call out for feedback:
I've chosen to disallow configuring the spacing supports for a mix of individual and axial sides -- eg no configuring padding for
verticalandleft. I think this would lead to confusion with naming, setting sensible defaults, etc. I disallow this by logging a warning when an invalid configuration is detected, and disabling the feature. I'm not aware of and couldn't find any other examples of block support configuration errors, so would love feedback here.This PR makes use of the existing
splitOnAxisfeature of theBoxControlintroduced in BoxControl: Add support for grouped directions (vertical and horizontal controls) #32610. For the Button block, grouped 'horizontal' and 'vertical' controls will render when the controls are unlinked, as opposed to four individual controls. I want to call this out since the linking/unlinking for Button here is different compared to other blocks.How has this been tested?
Test Button Block
Test Block Supports
Test configuring the support for all sides by default
Update
button/block.jsonto enable padding for all sides by changing it to:Test in the editor that all sides can be controlled individually when the controls are unlinked.
Test configuring arbitrary individual sides still works
Update
button/block.jsonto enable padding for arbitrary sides:Test that only the supported sides are able to be controlled when the controls are unlinked.
Test that invalid mixes of axial and individual sides are not allowed
Update
button/block.jsonto enable padding for an invalid mix of axial/individual sides:Test that a warning is logged and the padding feature is disabled; UI should not display.
Test margin support
Add margin support to the button block and repeat all tests. You will need to make sure that your theme enables margin support for the button block in order to do this.
Screenshots
button-padding.mov
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).