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.

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

Add Layout controls to children of Flex layout blocks #45364

Merged
merged 21 commits into from
Nov 25, 2022
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8e523f2
Add Layout controls to children of Flex layout blocks
tellthemachines Oct 28, 2022
3c1413c
Child layout styles on the editor
tellthemachines Nov 1, 2022
36fe32c
Try to get it working on server
tellthemachines Nov 1, 2022
18e3d9c
Fix style generation
tellthemachines Nov 1, 2022
d52ee1f
Fix classname logic
tellthemachines Nov 2, 2022
b23a665
Add hug option and change names to accommodate vertical orientation
tellthemachines Nov 2, 2022
9dd7e55
Don't trash existing styles
tellthemachines Nov 2, 2022
c4d96c9
Merge layout panels
tellthemachines Nov 3, 2022
c2d0cb4
Move controls to dimensions panel
tellthemachines Nov 17, 2022
02705ca
Use VStack for margins.
tellthemachines Nov 21, 2022
99b98e4
Change "hug" label and add help text.
tellthemachines Nov 21, 2022
8b66704
Wipe fixed width when other setting is chosen.
tellthemachines Nov 22, 2022
75d55bd
Update doc comment.
tellthemachines Nov 22, 2022
22ed793
Try parent layout logic in native BlockEdit
tellthemachines Nov 22, 2022
760e926
Add sizing controls to transformed Rows and Stacks
tellthemachines Nov 22, 2022
92d3bb0
Failsafe if parent layout doesn't exist
tellthemachines Nov 22, 2022
95d35f1
Fixed controls not rendering for certain block types
tellthemachines Nov 22, 2022
d11ff04
Actual 40px size
tellthemachines Nov 22, 2022
0065be1
Make opt-in a default property of layout support
tellthemachines Nov 24, 2022
1173722
Fix error when block doesn't exist
tellthemachines Nov 25, 2022
fabea58
Update lib/block-supports/layout.php
tellthemachines Nov 25, 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
73 changes: 68 additions & 5 deletions lib/block-supports/layout.php
Original file line number Diff line number Diff line change
Expand Up @@ -316,13 +316,61 @@ function gutenberg_get_classnames_from_last_tag( $html ) {
* @return string Filtered block content.
*/
function gutenberg_render_layout_support_flag( $block_content, $block ) {
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] );
$support_layout = block_has_support( $block_type, array( '__experimentalLayout' ), false );
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] );
$support_layout = block_has_support( $block_type, array( '__experimentalLayout' ), false );
$has_child_layout = isset( $block['attrs']['style']['layout']['selfStretch'] );

if ( ! $support_layout ) {
if ( ! $support_layout
&& ! $has_child_layout ) {
return $block_content;
}

$outer_class_names = array();

if ( $has_child_layout && ( 'fixed' === $block['attrs']['style']['layout']['selfStretch'] || 'fill' === $block['attrs']['style']['layout']['selfStretch'] ) ) {

$container_content_class = wp_unique_id( 'wp-container-content-' );

$child_layout_styles = array();

if ( 'fixed' === $block['attrs']['style']['layout']['selfStretch'] && isset( $block['attrs']['style']['layout']['flexSize'] ) ) {
$child_layout_styles[] = array(
'selector' => ".$container_content_class",
'declarations' => array(
'flex-shrink' => '0',
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, this is an interesting one. I assume this is to attempt to preserve what a user has chosen?

Will we always want to prevent the element from shrinking? That is, if someone sets a large value for flex-basis (e.g. 900px) I was wondering if we'll want to preserve that in smaller viewports where it'll cause an overflow, or in desktop viewports where it'll also overflow contentSize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if we set a fixed size we do want to prevent it from shrinking. That's touched upon in this comment, but also if we don't set flex-shrink there's no way to make sure the block actually sticks to the defined size 😅
As users are deliberately setting values, I think it's fine for them to overflow their containers where the values are too large.

Copy link
Contributor

Choose a reason for hiding this comment

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

As users are deliberately setting values, I think it's fine for them to overflow their containers where the values are too large.

Ah, I see your point! It's a tricky one to work out the desired behaviour here (whether to honour what the user is attempting to do, or try to make sure they don't break the layout). I think I probably lean more toward preventing them from overflowing the container, because otherwise it breaks outside of the content size / wide size logic, which could be unexpected, and any width that's set greater than 400px would overflow most phone's viewports in a vertical orientation. Here's a paragraph set to 500px on desktop and mobile:

Desktop Mobile
image image

From a quick look at the Column block's ad hoc width control, it appears that it currently doesn't include a flex-shrink: 0 rule, so it currently ensures that Columns blocks don't break out.

Either way, though, since these CSS rules aren't baked into post content, it looks like it'll be a simple rule to change if we want to 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting balance to strike on this one. In general, it seems like we try to start from a position of encouraging good design choices by default, then move towards honouring user choices. Following that, I'd lean a little more toward not allowing the broken layout first, then honouring explicit choices second.

That's only my naive two cents though, so take it with a grain of salt 🤷‍♂️

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'm not sure it's helpful to think of this situation as a "broken layout", because people might want to create a horizontally scrolling page, and currently there's no way to do so in the site editor. It's easy enough to undo a change, and obvious enough that a horizontal scroll is happening if the user does add a bunch of fixed widths by accident, that we should let it happen.

Also, as I said before, there's no way of making this feature work properly without the flex-shrink in place. If we show a control to fix the width of a block, and that control doesn't actually fix it, that's a much more broken experience than allowing users the possibility of making their page scroll horizontally.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a really interesting argument in both directions here (whether to honour the user's explicit choice, or attempt to do a fuzzy approximation). With other width controls (e.g. Columns or Search block), it looks like we currently don't allow overflowing the container? The search block sets the width value explicitly, but also has max-width: 100% so it doesn't overflow.

In terms of which user intent to prioritise, I'd be curious to hear thoughts from @jasmussen on this one — for a bit of history, there's been prior attempts to land a width block support, and the responsive behaviour was flagged on an earlier attempt here: #32502 (comment), and there's a subsequent open issue for responsive / intrinsic web design and how it relates to block controls in: #34641.

Whichever way we prioritise, I think it'd be good to figure out how we can support the one we haven't prioritised. E.g. if we went with the fuzzy approach, how do we then allow folks to intentionally create horizontally scrolling layouts, and alternately, if we honour the explicit pixel width, how do we allow folks to set a larger size that then shrinks in smaller viewports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was @jasmussen's recommendation here to let explicit widths break out of their container 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good catch, thanks @tellthemachines, I missed that one! That's a more recent comment so more likely captures the current thinking 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, it's always good to poke at stuff anyway and see how we can improve it!

I think in this case the fuzzy approach would add too much complexity for the value it might bring: removing the flex-shrink would stop the setting from working as expected, so we'd probably have to do some calculation of parent widths or something horrible like that 😬

Also, given we already have Columns with the width constraints, it makes sense to have this behave differently, so folks can use Columns for a content-width-respecting layout and Row for more freestyle options.

I'm actually pretty excited to finally have a way to create fixed width, horizontally scrolling pages tbh 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

Great points, and there's nothing stopping us from adding a fluid or fuzzy option as an additional control alongside fixed in a follow-up at some point to handle the case where someone wants to set a target width that gets either shrunk or clampified 👍

'flex-basis' => $block['attrs']['style']['layout']['flexSize'],
'box-sizing' => 'border-box',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This rule isn't being applied on the front end. Is it being removed by an overly strict CSS sanitiser?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, it looks like it isn't covered by safecss_filter_attr, which the style engine uses to filter CSS.

Should we add another safe_style_css filter for 6.2, like we did for 6.1 in this file? (e.g. add a blocks.php file to lib/compat/wordpress-6.2 and a new gutenberg_safe_style_attrs_6_2 function)

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 guess we'll have to 😅 but perhaps let's leave that for a follow-up PR.

),
);
} elseif ( 'fill' === $block['attrs']['style']['layout']['selfStretch'] ) {
$child_layout_styles[] = array(
'selector' => ".$container_content_class",
'declarations' => array(
'flex-grow' => '1',
),
);
}

gutenberg_style_engine_get_stylesheet_from_css_rules(
$child_layout_styles,
array(
'context' => 'block-supports',
'prettify' => false,
)
);

$outer_class_names[] = $container_content_class;

}

// Return early if only child layout exists.
if ( ! $support_layout && ! empty( $outer_class_names ) ) {
$content = new WP_HTML_Tag_Processor( $block_content );
$content->next_tag();
$content->add_class( implode( ' ', $outer_class_names ) );
return $content;
tellthemachines marked this conversation as resolved.
Show resolved Hide resolved
}

$block_gap = gutenberg_get_global_settings( array( 'spacing', 'blockGap' ) );
$global_layout_settings = gutenberg_get_global_settings( array( 'layout' ) );
$has_block_gap_support = isset( $block_gap ) ? null !== $block_gap : false;
Expand Down Expand Up @@ -428,21 +476,36 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
}
}

$content_with_outer_classnames = '';

if ( ! empty( $outer_class_names ) ) {
$content_with_outer_classnames = new WP_HTML_Tag_Processor( $block_content );
$content_with_outer_classnames->next_tag();
foreach ( $outer_class_names as $outer_class_name ) {
$content_with_outer_classnames->add_class( $outer_class_name );
}

$content_with_outer_classnames = (string) $content_with_outer_classnames;
}

/**
* The first chunk of innerContent contains the block markup up until the inner blocks start.
* We want to target the opening tag of the inner blocks wrapper, which is the last tag in that chunk.
*/
$inner_content_classnames = isset( $block['innerContent'][0] ) && 'string' === gettype( $block['innerContent'][0] ) ? gutenberg_get_classnames_from_last_tag( $block['innerContent'][0] ) : '';

$content = new WP_HTML_Tag_Processor( $block_content );
$content = $content_with_outer_classnames ? new WP_HTML_Tag_Processor( $content_with_outer_classnames ) : new WP_HTML_Tag_Processor( $block_content );

if ( $inner_content_classnames ) {
$content->next_tag( array( 'class_name' => $inner_content_classnames ) );
foreach ( $class_names as $class_name ) {
$content->add_class( $class_name );
}
} else {
$content->next_tag();
$content->add_class( implode( ' ', $class_names ) );
foreach ( $class_names as $class_name ) {
$content->add_class( $class_name );
}
}

return (string) $content;
Expand Down
5 changes: 4 additions & 1 deletion packages/block-editor/src/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import BlockCrashBoundary from './block-crash-boundary';
import BlockHtml from './block-html';
import { useBlockProps } from './use-block-props';
import { store as blockEditorStore } from '../../store';

import { useLayout } from './layout';
export const BlockListBlockContext = createContext();

/**
Expand Down Expand Up @@ -130,6 +130,8 @@ function BlockListBlock( {
const { removeBlock } = useDispatch( blockEditorStore );
const onRemove = useCallback( () => removeBlock( clientId ), [ clientId ] );

const parentLayout = useLayout();

// We wrap the BlockEdit component in a div that hides it when editing in
// HTML mode. This allows us to render all of the ancillary pieces
// (InspectorControls, etc.) which are inside `BlockEdit` but not
Expand All @@ -148,6 +150,7 @@ function BlockListBlock( {
isSelectionEnabled={ isSelectionEnabled }
toggleSelection={ toggleSelection }
__unstableLayoutClassNames={ layoutClassNames }
__unstableParentLayout={ parentLayout }
/>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import BlockInvalidWarning from './block-invalid-warning';
import BlockMobileToolbar from '../block-mobile-toolbar';
import { store as blockEditorStore } from '../../store';
import BlockDraggable from '../block-draggable';
import { useLayout } from './layout';

const emptyArray = [];
function BlockForType( {
Expand Down Expand Up @@ -78,6 +79,8 @@ function BlockForType( {
),
] );

const parentLayout = useLayout();

return (
<GlobalStylesContext.Provider value={ mergedStyle }>
<BlockEdit
Expand All @@ -99,6 +102,7 @@ function BlockForType( {
onDeleteBlock={ onDeleteBlock }
blockWidth={ blockWidth }
parentBlockAlignment={ parentBlockAlignment }
__unstableParentLayout={ parentLayout }
/>
<View onLayout={ getBlockWidth } />
</GlobalStylesContext.Provider>
Expand Down
20 changes: 15 additions & 5 deletions packages/block-editor/src/components/inner-blocks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { useViewportMatch, useMergeRefs } from '@wordpress/compose';
import { forwardRef } from '@wordpress/element';
import { useSelect } from '@wordpress/data';
import {
getBlockSupport,
getBlockType,
store as blocksStore,
__unstableGetInnerBlocksProps as getInnerBlocksProps,
Expand Down Expand Up @@ -74,27 +75,33 @@ function UncontrolledInnerBlocks( props ) {
templateInsertUpdatesSelection
);

const context = useSelect(
const { context, name } = useSelect(
( select ) => {
const block = select( blockEditorStore ).getBlock( clientId );

// This check is here to avoid the Redux zombie bug where a child subscription
// is called before a parent, causing potential JS errors when the child has been removed.
if ( ! block ) {
return;
return {};
}

const blockType = getBlockType( block.name );

if ( ! blockType || ! blockType.providesContext ) {
return;
return {};
}

return getBlockContext( block.attributes, blockType );
return {
context: getBlockContext( block.attributes, blockType ),
name: block.name,
};
},
[ clientId ]
);

const { allowSizingOnChildren = false } =
getBlockSupport( name, '__experimentalLayout' ) || {};

// This component needs to always be synchronous as it's the one changing
// the async mode depending on the block selection.
return (
Expand All @@ -103,7 +110,10 @@ function UncontrolledInnerBlocks( props ) {
rootClientId={ clientId }
renderAppender={ renderAppender }
__experimentalAppenderTagName={ __experimentalAppenderTagName }
__experimentalLayout={ __experimentalLayout }
__experimentalLayout={ {
...__experimentalLayout,
allowSizingOnChildren,
} }
wrapperRef={ wrapperRef }
placeholder={ placeholder }
/>
Expand Down
172 changes: 172 additions & 0 deletions packages/block-editor/src/hooks/child-layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
/**
* WordPress dependencies
*/
import {
__experimentalToggleGroupControl as ToggleGroupControl,
__experimentalToggleGroupControlOption as ToggleGroupControlOption,
__experimentalUnitControl as UnitControl,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import useSetting from '../components/use-setting';

function helpText( selfStretch ) {
switch ( selfStretch ) {
case 'fill':
return __( 'Stretch to fill available space.' );
case 'fixed':
return __( 'Specify a fixed width.' );
default:
return __( 'Fit contents.' );
}
}

/**
* Inspector controls containing the child layout related configuration.
*
* @param {Object} props Block props.
* @param {Object} props.attributes Block attributes.
* @param {Object} props.setAttributes Function to set block attributes.
* @param {Object} props.__unstableParentLayout
*
* @return {WPElement} child layout edit element.
*/
export function ChildLayoutEdit( {
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
attributes,
setAttributes,
__unstableParentLayout: parentLayout,
} ) {
const { style = {} } = attributes;
const { layout: childLayout = {} } = style;
const { selfStretch, flexSize } = childLayout;

return (
<>
<ToggleGroupControl
size={ '__unstable-large' }
label={ childLayoutOrientation( parentLayout ) }
value={ selfStretch || 'fit' }
help={ helpText( selfStretch ) }
onChange={ ( value ) => {
const newFlexSize = value !== 'fixed' ? null : flexSize;
setAttributes( {
style: {
...style,
layout: {
...childLayout,
selfStretch: value,
flexSize: newFlexSize,
},
},
} );
} }
isBlock={ true }
>
<ToggleGroupControlOption
key={ 'fit' }
value={ 'fit' }
label={ __( 'Fit' ) }
/>
<ToggleGroupControlOption
key={ 'fill' }
value={ 'fill' }
label={ __( 'Fill' ) }
/>
<ToggleGroupControlOption
key={ 'fixed' }
value={ 'fixed' }
label={ __( 'Fixed' ) }
/>
</ToggleGroupControl>
{ selfStretch === 'fixed' && (
<UnitControl
size={ '__unstable-large' }
style={ { height: 'auto' } }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid the inline style at all? There were some recent efforts to avoid these on UnitControls and controls in general.

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'm afraid we can't remove it until #45877 is fixed, otherwise the input blows out to full parent height:

Screenshot 2022-11-22 at 10 32 20 am

size={ '__unstable-large' } doesn't work on UnitControl either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid we can't remove it until #45877 is fixed, otherwise the input blows out to full parent height:

In previous instances where the style was still required, I have been requested to use a CSS class instead.

size={ '__unstable-large' } doesn't work on UnitControl either.

Are you only referring to the UnitControl here?

The size prop with __unstable-large is already in use and appears to work fine for the following components' UnitControls:

  • border radius control
  • spacing sizes control
  • min-height
  • and several others that pass through the size prop such as BorderBoxControl, FontSizePicker etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This UnitControl. I've gone with @mirka 's recommendation for now; anything else should be fine to address as a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification 👍

These controls still need to be 40px tall (as per this confirmation) before landing this though, so they are consistent with the other controls in the panel.

Copy link
Contributor

Choose a reason for hiding this comment

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

This works for me regarding fixing the control heights:

Example diff
diff --git a/packages/block-editor/src/hooks/child-layout.js b/packages/block-editor/src/hooks/child-layout.js
index 6ae08b221f..e29e1d9ac0 100644
--- a/packages/block-editor/src/hooks/child-layout.js
+++ b/packages/block-editor/src/hooks/child-layout.js
@@ -63,6 +63,7 @@ export function ChildLayoutEdit( {
 					} );
 				} }
 				isBlock={ true }
+				size={ '__unstable-large' }
 			>
 				<ToggleGroupControlOption
 					key={ 'fit' }
@@ -82,6 +83,7 @@ export function ChildLayoutEdit( {
 			</ToggleGroupControl>
 			{ selfStretch === 'fixed' && (
 				<UnitControl
+					size={ '__unstable-large' }
 					style={ { height: 'auto' } }
 					onChange={ ( value ) => {
 						setAttributes( {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, I was trying it instead of the auto height; it does seem to work in addition to it. Fixing now, thanks!

onChange={ ( value ) => {
setAttributes( {
style: {
...style,
layout: {
...childLayout,
flexSize: value,
},
},
} );
} }
value={ flexSize }
/>
) }
</>
);
}

/**
* Determines if there is child layout support.
*
* @param {Object} props Block Props object.
* @param {Object} props.__unstableParentLayout Parent layout.
*
* @return {boolean} Whether there is support.
*/
export function hasChildLayoutSupport( {
__unstableParentLayout: parentLayout = {},
} ) {
const {
type: parentLayoutType = 'default',
allowSizingOnChildren = false,
} = parentLayout;
const support = parentLayoutType === 'flex' && allowSizingOnChildren;

return support;
}

/**
* Checks if there is a current value in the child layout attributes.
*
* @param {Object} props Block props.
* @return {boolean} Whether or not the block has a child layout value set.
*/
export function hasChildLayoutValue( props ) {
return props.attributes.style?.layout !== undefined;
}

/**
* Resets the child layout attribute. This can be used when disabling
* child layout controls for a block via a progressive discovery panel.
*
* @param {Object} props Block props.
* @param {Object} props.attributes Block attributes.
* @param {Object} props.setAttributes Function to set block attributes.
*/
export function resetChildLayout( { attributes = {}, setAttributes } ) {
const { style } = attributes;

setAttributes( {
style: {
...style,
layout: undefined,
},
} );
}

/**
* Custom hook that checks if child layout settings have been disabled.
*
* @param {Object} props Block props.
*
* @return {boolean} Whether the child layout setting is disabled.
*/
export function useIsChildLayoutDisabled( props ) {
const isDisabled = ! useSetting( 'layout' );

return ! hasChildLayoutSupport( props ) || isDisabled;
}

export function childLayoutOrientation( parentLayout ) {
const { orientation = 'horizontal' } = parentLayout;

return orientation === 'horizontal' ? __( 'Width' ) : __( 'Height' );
}
Loading