Add padding support to Group block #24966
Conversation
youknowriad
added
[Feature] Blocks
youknowriad
requested review from
ajitbohra,
Soean and
talldan
as code owners
|
|
||
| .block-editor-block-list__layout > .wp-block:last-child { | ||
| margin-bottom: 0; | ||
| } |
There was a problem hiding this comment.
@jasmussen The idea here is that since we're defining the base margin above, we need to reset these margins for the first and last block of each container block.
There was a problem hiding this comment.
Without this, the padding is not applied properly in the group block (you'll notice an extra space on tthe editor)
There was a problem hiding this comment.
As can be seen in #24966 (comment), this isn't working for me. I have the following in my editor style:
figure,
p {
margin-top: 2.5rem;
margin-bottom: 2.5rem;
}
I'm still sort of worried about this being an awfully specific style rule that could have consequences we can't imagine. I don't have a strong opinion, insofar as I appreciate you trying to handle this so I wouldn't have to manually go to the last paragraph and specify a zero margin. I suppose we can try it, and remove it if it has side effects.
There was a problem hiding this comment.
I'm afraid this had unintended consequences.
The above happens because every nesting container has a last child that's the appender:
Unless, apparently, an adjacent block has been selected:
There was a problem hiding this comment.
I guess that's because the appender div is the "last child" so the margin reset is not applied. I wonder why this empty div is rendered and whether we can remove it.
There was a problem hiding this comment.
In this PR, I remove the rules: #25527 — I honestly think it's for the best.
There was a problem hiding this comment.
I'll take a look but I think there's still value in the rules. So I'd like to see if we can find a better solution.
| <BoxControlVisualizer | ||
| values={ attributes.style?.spacing?.padding } | ||
| showValues={ attributes.style?.visualizers?.padding } | ||
| /> |
There was a problem hiding this comment.
I think this can probably be absorbed by the hook and added automatically somehow. cc @ItsJonQ
There was a problem hiding this comment.
@ItsJonQ I'm not familiar with the attribute shape yet, can you clarify a bit what visualizers contain here?
There was a problem hiding this comment.
It'd be nice if it would be absorbed by the hook, otherwise we need to be more careful with any updates to BoxControlVisualizer. Also from its name, it feels like it shouldn't be a public API.
There was a problem hiding this comment.
@youknowriad Sure! I can expand on this a bit. The visualizers need to show in 2 ways:
- On value change (they flash) (via the
valuesprop) - On hover (they persist) (via the
showValuesprop, which arebooleanvalues)
Right now, state for showValues object is being communicated from the controls (in the sidebar) to the canvas via updating the style attribute.
It works, but I'd prefer a more streamlined way of coordinating this state - Ideally without using the data store and relying on a context + Provider wrapper.
Hope this helps!
There was a problem hiding this comment.
Yeah, I'm a bit concerned about the use of an attribute for that state since it's not a persistent state.
I wonder if we could just rely on local state if we move the rendering of the BoxControlVisualizer to the hook. Not sure thought.
| @@ -3,9 +3,6 @@ | |||
| */ | |||
|
|
|||
| .wp-block-group { | |||
| // Zero out the baseline margin that is set for every block in the editor. | |||
| margin-top: 0; | |||
| margin-bottom: 0; | |||
There was a problem hiding this comment.
I think this is not needed with my change here https://github.com/WordPress/gutenberg/pull/24966/files#r481034215
|
Size Change: +4.24 kB (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
|
Nice work. This is what I see:
Note that I'm applying a big padding by default on any group that has a background, in the theme shown here. But the padding correctly overrides that, including on the frontend. I'm seeing an error when customizing just one of the padding sides:
... but I'm seeing that also in Cover:
|
|
Yes, got the same error @jasmussen I'll have to investigate this one separately. |
|
I opened this separately to fix the unlinked padding values issues #25000 |
|
Hmm! I see the error as well. Taking a look at this 🤔 |
|
Update: It looks like there's something with the It works for me once I wrap the content in a function Tooltip( { children, text } ) {
if ( ! text ) return children;
return (
<BaseTooltip text={ text } position="top">
<div>{ children }</div> {/* here */}
</BaseTooltip>
);
}I'm not sure if anything changed 🤔 . My only guess is that something is happening with Tooltip's handling of |
|
Are we ok shipping this as is? |
closes #14747
This PR adds padding support to the Group block similar to how we do it for the Cover block.
Testing instructions
add_theme_support( 'experimental-custom-spacing' )