BlockGap: Add support for spacing presets #43296
Merged
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
andrewserong
added
[Feature] Design Tools
|
Size Change: +34 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
andrewserong
removed
the
[Status] In Progress
|
@andrewserong this works for me on the examples given, but breaks the gallery layout for some reason - gallery displays as a single column, just trying to work out why. |
ramonjd
reviewed
Aug 18, 2022
| @@ -1308,14 +1308,14 @@ protected function get_layout_styles( $block_metadata ) { | |||
| $block_gap_value = _wp_array_get( $block_type->supports, array( 'spacing', 'blockGap', '__experimentalDefault' ), null ); | |||
| } | |||
| } else { | |||
| $block_gap_value = _wp_array_get( $node, array( 'spacing', 'blockGap' ), null ); | |||
| $block_gap_value = static::get_property_value( $node, array( 'spacing', 'blockGap' ) ); | |||
There was a problem hiding this comment.
I think we need this also in get_styles_for_block here
Without it the body CSS rule with custom var isn't parsed:
body {
--wp--style--block-gap: var:preset|spacing|60;
}
There was a problem hiding this comment.
that will be what is breaking the gallery probably as it relies on --wp--style--block-gap as the fallback gap value if no block level gap is set
There was a problem hiding this comment.
Nice one, thanks for catching that! I'll update 👍
Good eyes! 🦅 👀
There was a problem hiding this comment.
I've updated in 2f1d687, and the Gallery block appears to be working now. I also removed the 0.5em fallback in that case, because it'll never be reached (the rule is only output if block gap is enabled, in which case, there will always be a real blockGap value because the root theme.json provides one)
It looks like the site editor was already parsing the value for the JS side of things, but let me know if you encounter any other issues!
There was a problem hiding this comment.
A question: should the following block style in theme.json have any effect?
"core/gallery": {
"spacing": {
"blockGap": "var:preset|spacing|80"
}
}There was a problem hiding this comment.
A question: should the following block style in theme.json have any effect?
No, the Gallery block isn't supported in global styles yet since it uses an ad hoc implementation for gap. But it's flagged in the Layout design tools consistency issue.
ramonjd
reviewed
Aug 18, 2022
| : getSpacingPresetCssVar( blockGapValue?.top ), | ||
| left: isValueString | ||
| ? getSpacingPresetCssVar( blockGapValue ) | ||
| : getSpacingPresetCssVar( blockGapValue?.left ), |
There was a problem hiding this comment.
This is not a blocker, and I'm not sure there's much to be done about it in this PR anyway.
When updating a single axial value we overwrite both top and left values:
.wp-block-social-links.is-layout-flex {
gap: var(--wp--preset--spacing--40) var(--wp--preset--spacing--20);
}The fallback value differs between the editor and the frontend:
.editor-styles-wrapper .wp-block-social-links.wp-container-37 {
gap: 0 7px;
}.wp-block-social-links.wp-container-10 {
gap: 0.5em 7px;
}
There was a problem hiding this comment.
Oh, good catch. Let's look at adding some consistency there in a follow-up.
There was a problem hiding this comment.
Just thinking about this a little more, unfortunately, because we're concatenating to the single gap property we do need to include some kind of fallback value there rather than letting the undefined side be undefined an inherit whatever's happening globally. Do you have an opinion on which one we should consolidate to? I'm a little torn 😅
There was a problem hiding this comment.
Ahhh I see. Maybe 0.5em since it's already in layout.php and will show up on the frontend regardless?
I don't know either!
There was a problem hiding this comment.
Thanks @ramonjd, I was leaning slightly toward 0.5em anyway. In this case, I think the fallback should only be used in the output of the Flex layout, to match the behaviour in the server-rendering (I think!), so in 8bfbd94 I've updated the flex layout to call the code with passing in 0.5em — that way the fallback here is defined in the caller, so we can have other values for other potential future layout types.
Does that sound reasonable to you as a short-term fix?
There was a problem hiding this comment.
Sounds great, thanks for doing that!
| @@ -1308,14 +1308,14 @@ protected function get_layout_styles( $block_metadata ) { | |||
| $block_gap_value = _wp_array_get( $block_type->supports, array( 'spacing', 'blockGap', '__experimentalDefault' ), null ); | |||
| } | |||
| } else { | |||
| $block_gap_value = _wp_array_get( $node, array( 'spacing', 'blockGap' ), null ); | |||
| $block_gap_value = static::get_property_value( $node, array( 'spacing', 'blockGap' ) ); | |||
There was a problem hiding this comment.
A question: should the following block style in theme.json have any effect?
"core/gallery": {
"spacing": {
"blockGap": "var:preset|spacing|80"
}
}
|
Testing well for me with gallery now in site editor, post editor and front end. Happy to give it another test when you have worked through @ramonjd feedback. |
…p entered, in flex layout
ramonjd
reviewed
Aug 18, 2022
| $gap_sides = is_array( $gap_value ) ? array( 'top', 'left' ) : array( 'top' ); | ||
|
|
||
| foreach ( $gap_sides as $gap_side ) { | ||
| $process_value = is_string( $gap_value ) ? $gap_value : _wp_array_get( $gap_value, array( $gap_side ), $fallback_gap_value ); |
There was a problem hiding this comment.
It looks like _wp_array_get is returning null for null values.
So, an incoming $gap_value of array(2) { ["top"]=> NULL ["left"]=> string(4) "38px" }
Will result in a $combined_gap_value of 38px.
Steps to reproduce:
- Insert social links block
- Add a block gap value for one side only.
The editor produces the following CSS:
gap: var(--wp--preset--spacing--20) 92px;The frontend
gap: 38px;There was a problem hiding this comment.
Sorry, ignore this. I refreshed the editor and I can't reproduce any more.
There was a problem hiding this comment.
Okay, cool. We can always do more detailed testing with this behaviour once we've got the spacing controls in there, I imagine it'll make testing a little easier since we'll be doing less manual fiddling with markup values 🤔
Thanks for all the testing here!
andrewserong
deleted the
update/block-gap-output-to-support-spacing-presets
branch
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.
What?
This is an alternative PR to #43076 to see if we can add support for spacing presets to the current blockGap output, without having to tie the implementation to the style engine work.
The objective is that any gap value stored in the form
var:preset|spacing|20will be output in a form such asvar(--wp--preset--spacing--20)in all situations where blockGap is output:Note: This PR does not add in a UI control for adjusting spacing presets — that will be covered in a separate PR. For now, this PR hones in on ensuring that the preset values can be used as valid values in blockGap output.
Why?
While working on #43076, it occurred to me that the changes required involve making tweaks to the style engine that are slightly beyond the scope of implementing the desired feature. In order to see if we can come up with a smaller changeset and a safer PR to merge, I thought it worth exploring to see if hacking in the CSS variable parsing into blockGap output is viable.
How?
static::get_property_valueto let the class do the CSS variable parsinglayout.php, borrow simplified logic from the style engine and include it inline in order to unwrap gap values into avar()valuegetSpacingPresetCssVarfunction to ensure gap values are parsed for CSS variables.Testing Instructions
1. Test root blockGap value works correctly in theme.json
In
theme.json, setstyles.spacing.blockGapto"var:preset|spacing|60", and check that this spacing value is output correctly in the site editor, post editor for default spacing between paragraphs, and as the default spacing on the site frontend.2. Test block level blockGap value works correctly in theme.json
In
theme.jsonset blockGap values for social icons and columns blocks:Then, test that the spacing is rendered correctly in the post editor, site editor, and on the site front end.
Here is some test markup
3. Test block level blockGap value works in post content
Because we haven't (yet) rolled out the preset-based spacing controls to blockGap, we can't edit this via the UI, but we can manually construct post content that uses the presets. Below is some sample markup, that when inspected in the editor and on the site's front end, should reveal that the parsing is working correctly:
In the above markup, ensure that the Social Icons, Columns, and Group blocks all use
var()rules for their gaps. (In the case of Group, look atmargin-block-starton the second child of the Group block to confirm)To recreate this manually, add blocks such as Social Icons or Columns, add a blockGap value, then go to the code editor view and replace the
pxbased values with a string in the formvar:preset|spacing|20where20is a valid preset slug (e.g.40,60, etc)Screenshots or screencast