Add padding and margin support to the Image block #43966
Conversation
glendaviesnz
added
[Type] Enhancement
glendaviesnz
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Thanks for picking this one up @glendaviesnz! It's generally working pretty well for me (and with images within a gallery block), but I was wondering what the expected behaviour would be in relation to the border support (introduced in #31366 by @aaronrobertshaw). With this PR applied, it currently applies padding to the wrapper figure element, so border appears within it. I was wondering if users would expect the padding to be inside the border (e.g. applied to the
I could image folks potentially wanting padding on either the |
hmm, yeh, good question, @aaronrobertshaw may have some thoughts based on the discussions that went on around the border support? |
|
As far as applying the spacing supports to the block wrapper this is working as advertised for me (including in the gallery).
@andrewserong does raise an interesting question though. One I don't have the answer to. Perhaps we could seek out some opinions from theme authors on this? I could definitely see something like the following being desirable.
|
The only discussions really around the image borders were that it shouldn't be on the root |
|
If we didn't adopt background color support for the image block in the future, could we consider margin to achieve the same goal as padding? Then skip serialization of spacing supports and apply margin to the wrapper and padding to the inner Alternatively, adding a toggle to set where the padding is applied could work for individual blocks. At the theme.json/global styles level, the styles could be generated for the wrapper and if a theme author wishes to apply padding to the img that could be done via the Elements API (we might have to expose These options might all be too messy given how complex this block is already. |
|
Yeh, I think it needs to be applied to the |
|
@andrewserong, @aaronrobertshaw I have updated this to skip serialization so we can apply padding to the image instead of the figure and I think it now works more like what people will expect, particularly if border is applied. image-padding.mp4 |
glendaviesnz
force-pushed
the
add/padding-margin-support-to-image-block
branch
from
357231a
to
516a0db
Compare
|
Size Change: +193 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is working pretty well for me @glendaviesnz, and the padding / margins get set where expected at the individual block level:
The main issue I ran into is that in global styles, the padding still gets applied to the outer wrapper, so there's a mismatch there:
What's the expected behaviour in global styles?
Other than that, the change here now grabs the padding and margin styles and applies them directly in the edit and save components, stripping away the remaining parts of the style object. E.g. whatever was in useBlockProps.save( ... ).style is overridden. This doesn't appear to cause any issues with the existing supports that are opted-into (and Duotone appears to still work well with custom values), however it means that if we were to opt-in to other block supports, their output would be stripped. As an example, if we switch supports.color.background to true and select a custom color (not a color preset) then with this PR applied, it doesn't work, and I suspect it'd be the same with Typography supports if we were to opt into them.
I imagine that's probably not really a blocker for landing this PR, given that it could be addressed separately when we go to do additional block supports.
So, I think that leaves the the global styles behaviour as the only blocker?
Also, would it be worth adding some block markup test fixtures for padding and margin to capture the state of the output for this?
|
@andrewserong we could do this to avoid overwriting any future blockprop styles? I am still looking at the global styles issue. |
Nice one, that looks good and straightforward to me, since the spacing props are already removed from the style object given that we're skipping serialization 👍 |
|
@andrewserong this change makes the padding work in global styles ... but it also applies the margin to the |
Ah, dang it, yes you're right. It looks like the feature level selectors only go to the overall feature group, rather than the individual spacing property 🤔 |
|
It's tough to know which direction to go in with this one, but one potential clue is that the PHP tests are now failing because the markup output no longer has the container based margin rule of |
@andrewserong I think I am a bit reluctant to introduce a divergence in behaviour between global styles and user block styles in order to get this into 6.1 as then we are probably stuck with that divergence. I think it might be better to explore extending the custom selectors to allow subfeature targeting, which obviously isn't going to happen before code freeze, but might be a better long term solution for this - what do you think? |
Absolutely, I was just thinking it feels like it'd be a bit awkward to attempt to land this PR in time. It probably needs a little more breathing room to play around with the idea of subfeature targeting like you mention 👍 Thanks for all the work today to get this as close as it could be, though! |
I'd be happy to take a run at this post 6.1 if you'd like given my previous dabbling in that area. I could see this potentially being useful for color supports. Maybe a block might wish to apply a background color to the wrapper but text color to a specific inner element. Typography supports might wish to selectively apply text-decoration. The recent search block adoption of typography needed to apply text-decoration to all inner elements except the input. |
|
Going to close for now until some issues around image captions are resolved elsewhere |
Related:
What?
Add padding and margin support to the Image block.
Why?
To create consistency across blocks.
How?
Added the relevant block supports in block.json
Testing Instructions
Screenshots or screencast
image-margin.mp4