Video: add spacing block supports #43365
Conversation
ramonjd
added
[Type] Enhancement
ramonjd
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
@apeatling Thanks for testing. The styles are printed out in the CSS, but it looks like the layout styles
Not sure how to tackle it yet, maybe there has to be an exception in layout.php if it detects these margin side values. cc @andrewserong for the usual sanity check 😄 |
|
Looks like it's coming from the theme.json layout rules printed out here: Naive way forward might be to check for existing values and compare with $declarations rules, but given that there could be any rule in there, it's far from bulletproof |
|
Oh, good discussion here! Margin is a tricky one — from the testing I did on a couple of PRs yesterday, I think it's pretty risky to attempt to reduce the specificity of the spacing between blocks in order for the From my perspective, I think the spacing of the wrapper that a block is in should take precedence over block-level-in-theme.json margin, and block level in individual block in post content should take precedence over the wrapper block's spacing. Otherwise we risk breaking how block gap works, as there's an inherent friction between the concepts of these two features. One of the workarounds that folks can use if they want the I suppose if I were to pick between the two, I'd pick margin to behave slightly unpredictably (as that is kinda how margin tends to work?) rather than trying to get the block spacing feature to work around it. Another thing to keep in mind is that left/right margin is already overwritten by alignment styles anyway, so there's precedent for margin only working in certain circumstances. What do you think? Is there another avenue that either of you think we should pursue for how the layout spacing works here? |
Thanks for the input @andrewserong After having a look at the code I think I agree 😄 It's a bit of a tangled web. Do you think we could get this and the spacer PR in given that block supports in the editor work?
I tried this before, but couldn't get around the layout styles:
|
Yes, it's great that we've found this edge case, but I don't think it should block these opt-in PRs, personally.
Ah, good catch — I think in another PR I must have been testing with a block that was in a Row or Stack, possibly, that doesn't have those spacing rules. I reckon it might be worth us opening an issue to see if it's worth coming up with a solution for the Group block — possibly an "unset" setting for spacing / block gap, or a layout type that is "no layout"? At any rate, it'd be good to look into separately to verify whether it's something that in practice folks will use, or if we're mostly encountering it here because we're testing these features in detail 🤔 Possibly a more likely real world use case for block-level margin in |
| @@ -77,6 +77,10 @@ | |||
| "supports": { | |||
| "anchor": true, | |||
| "align": true, | |||
| "spacing": { | |||
| "margin": true, | |||
| "padding": true | |||
There was a problem hiding this comment.
I noticed a slight issue with padding in TwentyTwentyTwo that I think is caused by the Video block using a width: 100% rule, in that the padding winds up being outside the 100% as opposed to part of it. Here's a Video block with padding next to a Group block with padding:
Shall we add a box-sizing: border-box rule as in the Audio block PR (It looks like most blocks with padding have the rule added, I think, along with a comment explaining why the rule was required) #43351 (review)
There was a problem hiding this comment.
Thanks for finding that. TIL
I'll add the rule and retest. 🙇
There was a problem hiding this comment.
Looks like that did the trick. Thank you!!!!
Thanks @andrewserong I'll add a TODO note for myself to make an issue today or next week. |
|
Size Change: +47 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
add/video-block-spacing-support
branch
from
269991e to
cb36b9d
Compare
Related:
What?
Enabling spacing support for the Video block
Why?
To create consistency across blocks.
How?
Adding the relevant block supports in block.json
Testing Instructions
2022-08-18.19.33.21.mp4