Skip to content
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.

Sign up for GitHub

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

Layout: Add default fallback gap value in block.json, and set to 2em for Columns blocks #41098

Merged
merged 6 commits into from
May 20, 2022
Merged

Layout: Add default fallback gap value in block.json, and set to 2em for Columns blocks #41098

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
56152c6
Layout: Try adding a default fallback gap value in block.json
andrewserong May 17, 2022
a9dd741
Implement in the editor, too
andrewserong May 17, 2022
a2cc158
Try moving the default value to sit under the blockGap key in spacing…
andrewserong May 19, 2022
426f67a
Update doc comment
andrewserong May 19, 2022
e779a9f
Add explanatory comments
andrewserong May 19, 2022
7774b7c
Add __experimental prefix to default property.
andrewserong May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,11 @@ function gutenberg_register_layout_support( $block_type ) {
* @param boolean $has_block_gap_support Whether the theme has support for the block gap.
* @param string $gap_value The block gap value to apply.
* @param boolean $should_skip_gap_serialization Whether to skip applying the user-defined value set in the editor.
* @param string $fallback_gap_value The block gap value to apply.
*
* @return string CSS style.
*/
function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support = false, $gap_value = null, $should_skip_gap_serialization = false ) {
function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support = false, $gap_value = null, $should_skip_gap_serialization = false, $fallback_gap_value = '0.5em' ) {
$layout_type = isset( $layout['type'] ) ? $layout['type'] : 'default';

$style = '';
Expand Down Expand Up @@ -102,14 +103,14 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$style .= 'display: flex;';
if ( $has_block_gap_support ) {
if ( is_array( $gap_value ) ) {
$gap_row = isset( $gap_value['top'] ) ? $gap_value['top'] : '0.5em';
$gap_column = isset( $gap_value['left'] ) ? $gap_value['left'] : '0.5em';
$gap_row = isset( $gap_value['top'] ) ? $gap_value['top'] : $fallback_gap_value;
$gap_column = isset( $gap_value['left'] ) ? $gap_value['left'] : $fallback_gap_value;
$gap_value = $gap_row === $gap_column ? $gap_row : $gap_row . ' ' . $gap_column;
}
$gap_style = $gap_value && ! $should_skip_gap_serialization ? $gap_value : 'var( --wp--style--block-gap, 0.5em )';
$gap_style = $gap_value && ! $should_skip_gap_serialization ? $gap_value : "var( --wp--style--block-gap, $fallback_gap_value )";
$style .= "gap: $gap_style;";
} else {
$style .= 'gap: 0.5em;';
$style .= "gap: $fallback_gap_value;";
}

$style .= "flex-wrap: $flex_wrap;";
Expand Down Expand Up @@ -184,10 +185,12 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
$gap_value = $gap_value && preg_match( '%[\\\(&=}]|/\*%', $gap_value ) ? null : $gap_value;
}

$fallback_gap_value = _wp_array_get( $block_type->supports, array( 'spacing', 'blockGap', 'default' ), '0.5em' );

// If a block's block.json skips serialization for spacing or spacing.blockGap,
// don't apply the user-defined value to the styles.
$should_skip_gap_serialization = gutenberg_should_skip_block_supports_serialization( $block_type, 'spacing', 'blockGap' );
$style = gutenberg_get_layout_style( ".$class_name", $used_layout, $has_block_gap_support, $gap_value, $should_skip_gap_serialization );
$style = gutenberg_get_layout_style( ".$class_name", $used_layout, $has_block_gap_support, $gap_value, $should_skip_gap_serialization, $fallback_gap_value );
// This assumes the hook only applies to blocks with a single wrapper.
// I think this is a reasonable limitation for that particular hook.
$content = preg_replace(
Expand Down
12 changes: 9 additions & 3 deletions packages/block-editor/src/hooks/dimensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ const useIsDimensionsDisabled = ( props = {} ) => {
};

/**
* Custom hook to retrieve which padding/margin is supported
* Custom hook to retrieve which padding/margin/blockGap is supported
* e.g. top, right, bottom or left.
*
* Sides are opted into by default. It is only if a specific side is set to
Expand All @@ -162,7 +162,7 @@ const useIsDimensionsDisabled = ( props = {} ) => {
* @param {string} blockName Block name.
* @param {string} feature The feature custom sides relate to e.g. padding or margins.
*
* @return {Object} Sides supporting custom margin.
* @return {?string[]} Strings representing the custom sides available.
*/
export function useCustomSides( blockName, feature ) {
const support = getBlockSupport( blockName, SPACING_SUPPORT_KEY );
Expand All @@ -172,7 +172,13 @@ export function useCustomSides( blockName, feature ) {
return;
}

return support[ feature ];
if ( Array.isArray( support[ feature ] ) ) {
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
return support[ feature ];
}

if ( support[ feature ]?.sides ) {
return support[ feature ].sides;
}
}

/**
Expand Down
14 changes: 11 additions & 3 deletions packages/block-editor/src/layouts/flex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
arrowDown,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about flow.js? no fallback support there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that too when I went to work on this feature — the Flow layout currently doesn't use any fallback value at all, so I didn't inject one where there isn't one already (both in layout.php and flow.js). From looking over the code again, it appears that blockGap styles in the Flow layout are only ever output if the blockGap has been opted into, in which case there'll always be a value, so a fallback value will never be reached. E.g. in the following block:

gutenberg/packages/block-editor/src/layouts/flow.js

Line 168 in 6de1698

if ( hasBlockGapStylesSupport ) {

It was the Flex layout that currently has a 0.5em hard-coded fallback, and where I think this feature is useful based on the current implementations.

But, please do let me know if I missed something here! 🙂

} from '@wordpress/icons';
import { Button, ToggleControl, Flex, FlexItem } from '@wordpress/components';
import { getBlockSupport } from '@wordpress/blocks';

/**
* Internal dependencies
Expand Down Expand Up @@ -109,14 +110,21 @@ export default {
save: function FlexLayoutStyle( { selector, layout, style, blockName } ) {
const { orientation = 'horizontal' } = layout;
const blockGapSupport = useSetting( 'spacing.blockGap' );
const fallbackValue =
getBlockSupport( blockName, [
'spacing',
'blockGap',
'default',
] ) || '0.5em';

const hasBlockGapStylesSupport = blockGapSupport !== null;
// If a block's block.json skips serialization for spacing or spacing.blockGap,
// don't apply the user-defined value to the styles.
const blockGapValue =
style?.spacing?.blockGap &&
! shouldSkipSerialization( blockName, 'spacing', 'blockGap' )
? getGapCSSValue( style?.spacing?.blockGap, '0.5em' )
: 'var( --wp--style--block-gap, 0.5em )';
? getGapCSSValue( style?.spacing?.blockGap, fallbackValue )
: `var( --wp--style--block-gap, ${ fallbackValue } )`;
const justifyContent =
justifyContentMap[ layout.justifyContent ] ||
justifyContentMap.left;
Expand All @@ -143,7 +151,7 @@ export default {
${ appendSelectors( selector ) } {
display: flex;
flex-wrap: ${ flexWrap };
gap: ${ hasBlockGapStylesSupport ? blockGapValue : '0.5em' };
gap: ${ hasBlockGapStylesSupport ? blockGapValue : fallbackValue };
${ orientation === 'horizontal' ? rowOrientation : columnOrientation }
}

Expand Down
4 changes: 3 additions & 1 deletion packages/block-library/src/columns/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@
}
},
"spacing": {
"blockGap": true,
"blockGap": {
"default": "2em"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about an __experimental property?

Suggested change
"default": "2em"
"__experimentalDefault": "2em"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @adamziel, It looks like a new API would be a more controversial choice in the context of the WP 6.0 release. Regardless of how we apply that to wp/6.0 branch, this PR will require documentation changes as a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yes that's a great idea Adam, I'll update to add an experimental prefix 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the __experimental prefix in: 7774b7c

If this PR lands in time, @ramonjd and I are happy to write up documentation changes in a follow-up.

},
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
"margin": [ "top", "bottom" ],
"padding": true,
"__experimentalDefaultControls": {
Expand Down