-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block Support: Add gap block support feature #33991
Conversation
Size Change: +611 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
Based on feedback in #33812 (comment), I've refactored this to use a single CSS variable ( Next up, I'll look into server-rendering the value instead of storing in inline styles. Just a note here that this is still in progress, I'll update the PR description to match the current state of things later on today. |
Added in server-rendering in 3fea1eb along with skipping the serialization of the blockGap value in post content, which seems like it's going to work to get around issues with storing CSS variables in post content. So it seems promising so far! There are some outstanding issues with the approach I've gone with, which I'll look at cleaning up tomorrow along with adding more tests. |
474e5fc
to
c3f2081
Compare
Just noticed another bug to do with the difference between rendering within the editor and rendering server-side: it appears that the Edit component's inline styles aren't being rendered in the site editor. I'll take a look at what's going on. |
This issue is to do with rendering the gap support styles within the
An option to explore is to render the |
I've had a go at server-rendering an inline style in 842bbfe to resolve the issue of rendering in the |
I've updated this PR to tweak the regex slightly and add in some additional PHP tests. I've also removed gap from the function that applies the spacing support for dynamic blocks, as this gets handled anyway by the render_block callback for all blocks. I think once #33812 lands, this PR should be in a good place for reviews / testing. I'll continue to update this PR if and when that PR lands to remove the duplication that gets this PR working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this on top of trunk yet, but this is looking great!
- When configured, Gap settings appear by default in the Dimensions panel
- Tested changing the Gap value from the Dimensions panel
- Tested setting a Gap through theme.json
- Tested setting a Gap through global styles
- Gap values set per-block override global styles, which override theme.json
- Gap styles reflected correctly on the frontend, site editor, and block editor
- Tested that Gap works with different alignment and items justification values
The only slightly odd things I noticed are just Buttons-specific stuff and would be handled when adding the support to that block (the existing top/bottom margin you pointed out, and a styling regression on trunk). This is looking really nice!
Thanks for testing @stacimc! I've raised a PR to look into the styling regression in #34189. Once that's resolved, I'll give this a rebase and we can see about getting a final test/review.
I imagine there'll be a few wrinkles for implementing the gap support for particular blocks, but hopefully with the CSS variable approach, this will free us up to address the concerns of the individual blocks on their own PRs, like you suggest 🙂 |
Also tested according to instructions. Aside from the extra margins on buttons, which @stacimc already highlighted, it's working well for me. NICE! I also tried it out on other blocks as well. Here's the gallery block: It's going to be very useful for such patterns. Thank you! |
Thanks for the question @fwazeter, it looks like you've probably already read the linked issue, but for completeness sake — according to the theme.json v2 issue the current long-term plan appears to be to remove the |
Indeed, I was testing on Safari, what a weird bug. Curious to know if they're aware of it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @youknowriad, I appreciate the review and feedback! Since this PR adds in the block support without switching it on anywhere, I'll merge it in now and I can look at enhancements and a fix for Safari in follow-ups. 🙇 |
Description
Related to: #32366
Fixes: #29098
Note: this PR supersedes #32571 — it is essentially the same PR, but now based off trunk
This PR is a follow-up to the gap CSS variable approach for the layout support added in #33812. It proposes adding in a Gap block support feature for blocks that could use a Gap spacing control. Potential candidates for using this include:
To simplify things for this PR, we generate a single CSS variable (
--wp--style--block-gap
) for consistency with #33812. The idea at the moment is that we introduce a simple uniform gap control, and if we need to split out horizontal/vertical gap spacing, we can look into that further down the track / in follow-ups.The CSS variable is generated via server-rendering and injecting the style inline into rendered blocks. The reason for this is because arbitrary CSS variables are not able to be updated in post content by users that don't have the
unfiltered_html
capability (e.g. Author and Contributor roles). This is a similar kind of approach as to the Layout support. Additionally, styles are rendered inline instead of via a separatestyle
element so that the style will be rendered correctly in the context ofpost-content
blocks in FSE.To-dos
unfiltered_html
capabilityHow has this been tested?
PHP tests:
Switch on the block gap support
theme.json
file, undersettings.spacing
add the"blockGap": true
property. E.g. this is how my spacing settings look:packages/block-library/src/buttons/block.json
add thegap
"spacing" support. In my testing, the "supports" key of this file looks like this:packages/block-library/src/buttons/style.scss
at line 8 add the following to set the gap styles:Testing applying styles via theme.json
In a
theme.json
file you can specify gap styles in the following way (in this example, applied to the buttons block):Using the
blockGap
propertyIn the
theme.json
file, use:This should result in the following rendered styles on the front-end of the site:
Screenshots
This PR only adds the gap block support feature, and doesn't switch it on for any blocks by default, but the following screenshots are how it looks if applied to the Buttons block:
In Global styles:
Note that due to the Layout support also using the same CSS variable, in the above global styles screenshot, we can see the changed value affects the block's margin, too. This could be something for us to look into when applying the support to individual blocks.
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.js
files for terms that need renaming or removal).