File block: Add spacing support #45107
Conversation
|
|
carolinan
added
[Block] File
carolinan
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: +45 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you for putting this PR together @carolinan 👍
In general, it is testing pretty well for me however, I did hit a few issues.
✅ Padding & margin work for individual blocks
✅ Spacing supports values for individual blocks are reflected in both the editor and frontend
✅ Spacing supports work regardless of whether the file is an embedded PDF or not
❓ Default bottom margin is present but is overridden by constrained layout styles
✅ Padding can be configured via theme.json and Global Styles
❓ Top/bottom margin cannot be configured via theme.json or Global Styles
The main problem appears to be the layout styles overriding the block's, whether that is the default bottom margin or top and bottom margins configured via theme.json or global styles. In the case of the theme.json/global styles margins, they are specific enough in the editor to be applied but not on the frontend.
Unfortunately, I don't have the proper context around layout supports to offer much in the way of solutions to the issues noted above. @tellthemachines or @andrewserong are the layout experts. They may have encountered this before or have some ideas 🤞
P.S. One tiny nit, perhaps we should also note and explain the moving of the default margin style to the block.json, and the box-sizing reset, in the PR description.
|
WordPress 6.1.1 with Twenty Twenty-Three. Site editor: Front: Block editor:
Front, viewing the post with the single file block inside: The margin is not visible. |
|
Looks like #43404 might cover the problem with margin support styles being overridden by the layout styles. I'm wondering if we might be better served to omit the adoption of margin support until we are closer to a solution on that issue. |
|
Yes I think you are right, let's wait. |
carolinan
added
the
[Status] Blocked
|
Flaky tests detected in b8bb5c2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5642275721
|
There was a problem hiding this comment.
Thanks for keeping this PR updated @carolinan 👍
I've given it another look to see if we can unblock it but it does still appear to be blocked on two fronts.
- The layout support styles still overriding margin support styles, currently being worked on in:
- Classic themes without theme.json won't get the generated styles via the
__experimentalStylesprop in block.json causing a regression due to the removal of the default CSS style.
If padding support would be beneficial in the short term perhaps we could just omit the margin support for now and revert the __experimentalStyle change.
I'm also fine with leaving the PR on the back burner until someone really needs the spacing support.
| "__experimentalStyle": { | ||
| "spacing": { | ||
| "margin": { | ||
| "bottom": "1.5em" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Unfortunately, I don't think we can make this switch yet as it would be a regression for themes without a theme.json file due to the generated styles not being loaded for them.
See: #46818
There was a problem hiding this comment.
Thanks for mentioning this, and for linking through to the WIP work to fix layout conflicts with margins. Something that's also relevant here is that in the exploration by @tellthemachines over in #47858 the Columns block's default margin-bottom rule has its specificity reduced. The idea in that PR is that for layout rules to be able to have reduced specificity, we likely will also need all core blocks' default margin rules to be lower specificity too (e.g. wrapped in a :where() rule) so that low specificity layout rules can easily override it. I imagine we'd need to do that for the file block, too.
There was a problem hiding this comment.
Unfortunately, I don't think we can make this switch yet as it would be a regression for themes without a theme.json file due to the generated styles not being loaded for them.
There is a specific stylesheet loaded for them now though? The style should be added there.
There was a problem hiding this comment.
Here is where the classic.scss file was introduced.
#44334
https://developer.wordpress.org/reference/functions/wp_enqueue_classic_theme_styles/
In 6.2 it uses wp_theme_has_theme_json() to determine when the file is loaded.
There was a problem hiding this comment.
Thank you for trying out the classic.scss approach @carolinan
Unfortunately, I think things have changed a bit recently. #47858 landed which fixed the layout margin rule specificity. Part of that meant lowering the specificity of some blocks' CSS rules. This included the File block.
The current state of this PR means we would have the margin rule in 3 places (style.scss, classic.scss, and block.json).
I suspect we might be better off removing the addition of the new rule to classic.scss and the block.json via __experimentalStyle. The latter overrides the lower margin specificity required to work alongside layout supports.
There was a problem hiding this comment.
I don't think the layout margin rules are fixed, there are issues with two of the other blocks I have spacing PR's for.
There was a problem hiding this comment.
The "fixing" of margin rules was so that a block's global styles for margin could be applied. I believe the fix from #47858 is still working. It sounds like the interaction between the overall layout styles and top-level block margins is what you don't think is working correctly?
I'm about to start looking more closely at the Preformatted spacing PR.
The left/right margins not being applied for top-level blocks makes sense but I need to have a play around with the PR to see what's happening on the frontend. I'll comment further on that PR.
There was a problem hiding this comment.
Coming back to this one with fresh eyes, I still think we just need to remove the classic.scss style and this __experimentalStyle entry and we should be able to move ahead with this PR.
There was a problem hiding this comment.
This PR is pretty close 👍
Once we remove the new style defaults that override layout styles, I believe we'll be in a position where we can merge. Any further rough edges around margins within flex containers should be covered by #50825.
| .wp-block-file { | ||
| margin-bottom: 1.5em; | ||
| } | ||
|
|
There was a problem hiding this comment.
As noted in the __experimentalStyle thread, we'll need to remove this style from classic.scss to maintain the lower specificity default set in style.scss required by layout supports.
| "__experimentalStyle": { | ||
| "spacing": { | ||
| "margin": { | ||
| "bottom": "1.5em" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Coming back to this one with fresh eyes, I still think we just need to remove the classic.scss style and this __experimentalStyle entry and we should be able to move ahead with this PR.
There was a problem hiding this comment.
With the removal of the default styles overriding layout styles and work continuing on #50825, this is ready to land.
Thanks for your patience @carolinan 🙇
What?
Adds margin and padding block support to the file block.
Moves the default margin from style.scss to block.json to reduce duplicate styles:
Why?
For consistency.
How?
Testing Instructions
Create a new post or page.
Add two file blocks: one should be an embedded PDF file, the second any linked file.
Confirm that there is a dimension panel and that margin and padding are available but not enabled by default.
Confirm that the default bottom margin is present.
Test the margin and padding settings in the editor and front.
Note: The File block is not listed in Site Editor > Styles > Blocks but can be styled via theme.json.
Screenshots or screencast