Allow themes with theme.json to opt-out of block gap styles #34491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
Needs Dev Note
youknowriad
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
|
Note that I made it "opt-out" for now: you can to explicitly set
|
|
Size Change: -2.01 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
youknowriad
changed the title
|
I can confirm this works as intended. With the opt out in place:
Without the opt out, it's like trunk:
Ignore the wrong margin of the first block in the above GIF, that's tracked here: #34441. I love the block gap, it's vastly simplified margins in my theme to the point that I can have fewer margin rules overall, letting me focus in more important things. I look forward to I'm not opposed to an opt-out, though I do appreciate it being an opt-out rather than an opt-in. I would have almost certainly not discovered the simplicity this brought to my theme setup, had it been opt-in. |
7 tasks
andrewserong
reviewed
Sep 3, 2021
There was a problem hiding this comment.
Nice change @youknowriad, I think it's good adding an opt-out for folks who want to use it, and I agree with Joen that an opt-out is a good option for ensuring that we don't rely on having to discover the feature. Also it's easy enough for us to flip it the other way around, too, which is nice.
I added one tiny nit, and a question to make sure that I'm following how it works. But otherwise looks really good to me. Just noticed that it looks like it needs a rebase for the class-wp-theme-json-gutenberg.php file. Thanks!
youknowriad
force-pushed
the
add/opt-out-block-gap-settings
branch
from
19869d0 to
80d93f9
Compare
andrewserong
approved these changes
Sep 6, 2021
There was a problem hiding this comment.
Thanks for adding in this feature @youknowriad, the code changes looks good to me, and it's testing nicely in the editor and on the front end.
✅ With the setting switched to null the spacing renders correctly in the editor and on the front end using the real CSS values instead of the CSS variable.
✅ With the setting switched to true the spacing still renders correctly, but picks up and uses the --wp--style--block-gap CSS variable.
Just noticed in theme.json that we're defining "blockGap": false twice in that object, but other than that, this LGTM!
lib/theme.json
Outdated
| @@ -214,6 +214,7 @@ | |||
| "blockGap": false, | |||
| "customMargin": false, | |||
| "customPadding": false, | |||
| "blockGap": false, | |||
There was a problem hiding this comment.
Just noticed that this line is already set three lines up 😀
|
Per the feedback received recently about the unhandled use-cases (headings...), I'm making block gap opt-in for a start. Eventually we should switch it to opt-out before merging into Core. |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #33812 (comment) and the follow-up discussion, it has been brought up that some themes might want difference spacing after blocks in the "flow" layout depending on the block itself. We don't have a declarative solution for that yet in theme.json, so to let existing themes continue using the adhoc custom CSS approach, I'm adding a
blockGapsetting in this PR that if set tonullwould allow themes to opt-out of the block gap styles generation. Themes that opt-out will continue to work like classic themes, forcing developers to provide their own margins for the flow layout.Note
nullvalue. I expect that potentially other settings would need a similar behavior in the future.Testing instructions
Play with the blockGap setting in your theme.json:
nulldisables the block gap entirely (so even if you define a blockGap in styles, it has no effect)nullor omitting it entirely means the blockGap styles are enabled and you should see the effect of theblockGapstyle.