Preformatted: Add spacing support #45196
Conversation
carolinan
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: +65 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
carolinan
added
[Type] Enhancement
There was a problem hiding this comment.
Thanks for continuing to adopt block supports @carolinan, appreciate it 👍
This is pretty close to working well for me. Unfortunately, I hit similar issues to #45107.
✅ Spacing supports are applied in the editor for individual blocks
✅ Margins for top-level individual blocks get applied on the frontend
❓ The box-sizing reset isn't applied on the frontend. Makes padding impact block width incorrectly.
✅ Theme.json/global styles set padding works (ignoring the box-sizing issue noted above)
❓ Theme.json/global styles set margins are only applied in the editor
❓ When nested in a Group block, horizontal margins restrict the available width for the Preformatted block, while vertical margins appear to push outside the block instead. This occurs on both the editor and frontend for me.
It feels like spacing supports alongside layout support are now a bit broken. It's very likely I'm just missing something obvious. @tellthemachines or @andrewserong can you point us in the right direction to making this all behave?
Not sure if it helps, but there's an open issue in #43404 for the problem with the specificity of layout margin rules and how they behave alongside margins set in theme.json/global styles. Unfortunately, it doesn't appear yet to have an obvious fix, as reducing the specificity of the base layout rules then causes issues with styles that are shipped with the core blocks. Is the current behaviour any different to what was reported there? In general, I think margin is a challenging block support to reason about since its rules are shared with layout, so it's good when we opt in to it, for it to be not-exposed-by-default, as in this PR. |
carolinan
added
the
[Status] Blocked
|
Is it worth reducing the scope of this PR to just adding padding support, such that it can move forward while we wait for the fix for layout styles overriding margin? |
|
Sure, it is a small code change. |
|
Flaky tests detected in 73ff54b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5419684268
|
There was a problem hiding this comment.
Thanks for dusting this one off @carolinan 👍
While re-testing this PR I found a padding issue when the preformatted block has a background color. Global Styles and theme.json applied padding will work in the editor but the .wp-block-preformatted.has-background style overrides it on the frontend.
We might be able to get away here by wrapping the block's has-background style in :where to lower its specificity.
Also, given it's been a while, I believe the fix for the layout styles overriding margin supports was merged in #47858.
It might be worth re-adding that given we have to make changes here anyway. What do you think?
|
I did not double-check the margin on this block because I ran into issues with the margin on the comments pagination block #43905 (comment), I will check it now. |
|
I think that the default padding should only be enforced when a background color is added, if the theme |
|
Related: #37356 |
|
Using Twenty Twenty-Three.
|
There was a problem hiding this comment.
Thank you @carolinan for your effort and patience in working through the kinks in adopting spacing supports to blocks.
I think that the default padding should only be enforced when a background color is added, if the theme adds support for add_theme_support( 'wp-block-styles' ); ?
That would be the ideal case. This is a breaking change as noted on the linked issue however, so I don't think we can relocate those styles in this PR. It needs a wider solution.
When I enable the margin block support and adjust the blocks margin in the Styles sidebar, the following happens:
- Block editor: Only top and bottom margin works. Left and right margins are overwritten by:
- Front: None of the margins work:
The interactions between layouts and margin styles get a bit confusing at times. To me, the current behaviour is mostly making sense. As a top-level block, I'd expect the left/right margins to be overridden as they are.
For an individual block's styles, which are applied via inline styles, I would also expect those top/bottom margins to get applied. They do, and are consistent between the editor and frontend for me.
When applying a global styles margin for the Preformatted block type, I'd expect those to respect the bounds of a layout. So for a top-level block, the layout overrides the top margin of the first block and overrides the bottom margin for the last block. This is the behaviour I get when testing as well in both the editor and frontend.
Screen.Recording.2023-05-22.at.2.19.24.pm.mp4
One scenario where I did find a discrepancy though was when nesting the block within a container using a flex layout e.g. Row or Stack blocks. In the editor, the margins are applied but overridden on the frontend.
| Editor | Frontend |
|---|---|
|
|
@tellthemachines or @andrewserong perhaps you could provide some insight on to what extent layouts should override margins?
|
We need to try set aside our own knowledge. As a user, all that is known is that "the setting isn't doing what it says". They don't know anything about boundaries of the layout. |
Thanks for the ping! From a quick skim of the layout rules, I think it might be to do with the |
I've opened a WIP PR over in #50825 — it appears to mostly work pretty well, but unfortunately has some conflicts styles that are included in |
With constrained layout this is no longer an issue since #47858. I notice your code snippet still shows the old rule ( The issue with flex rules and margins is a different one. It would be good to make sure what is the desired behaviour there; there's a bit more about it in #44736. |
|
I have placed two preformatted blocks in the block editor. Global styles: I think this is unblocked, or do we wait for the flex issue? |
I don't think it's worth waiting for the flex issue, as it'll be fixed separately in #50825. In any case, it affects all blocks with margins, not specifically this one. |
carolinan
removed
the
[Status] Blocked
|
If this is unblocked, can someone approve it? 😸 |
There was a problem hiding this comment.
My apologies that this took so long to come back to, @carolinan 🙇
I believe the spacing supports for this block are behaving as expected and #50825 will take care of the remaining issues within flex containers.
As others have noted, I don't think the wait on #50825 needs to block this PR any further.
What?
Adds margin and padding to the preformatted text block.
Why?
For consistency, to allow users to add padding to the block.
How?
Testing Instructions
Make sure that the active theme supports spacing, for example in theme.json.
Create a new posts or page.
Add a preformatted text block.
Confirm that there is a dimension panel, and that the padding and margin settings are available but not enabled by default.
Test the padding and margin settings in the block editor, Site Editor Styles sidebar (global styles) and front.
Adjust the padding and margin via theme.json and confirm that it is applied in the editors and front.
Screenshots or screencast
preformatted-padding-stylebook.mov