-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Changes from 11 commits
8e523f2
3c1413c
36fe32c
18e3d9c
d52ee1f
b23a665
9dd7e55
c4d96c9
c2d0cb4
02705ca
99b98e4
8b66704
75d55bd
22ed793
760e926
92d3bb0
95d35f1
d11ff04
0065be1
1173722
fabea58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
'flex-basis' => $block['attrs']['style']['layout']['flexSize'], | ||
'box-sizing' => 'border-box', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, it looks like it isn't covered by Should we add another There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,168 @@ | ||
/** | ||
* 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 control panel containing the child layout related configuration | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This comment refers to a panel where it really returns a control. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh well spotted, thanks! |
||
* | ||
* @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 | ||
label={ childLayoutOrientation( parentLayout ) } | ||
value={ selfStretch || 'fit' } | ||
help={ helpText( selfStretch ) } | ||
onChange={ ( value ) => { | ||
setAttributes( { | ||
style: { | ||
...style, | ||
layout: { | ||
...childLayout, | ||
selfStretch: value, | ||
}, | ||
}, | ||
} ); | ||
} } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we also reset the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! Missed that somehow 😅 |
||
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 | ||
style={ { height: 'auto' } } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In previous instances where the style was still required, I have been requested to use a CSS class instead.
Are you only referring to the UnitControl here? The
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works for me regarding fixing the control heights: Example diffdiff --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( {
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' ); | ||
} |
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.
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 overflowcontentSize
?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.
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.
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.
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 to500px
on desktop and mobile: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 🙂
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.
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 🤷♂️
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'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.
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 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 hasmax-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.
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.
It was @jasmussen's recommendation here to let explicit widths break out of their container 😄
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.
Ah, good catch, thanks @tellthemachines, I missed that one! That's a more recent comment so more likely captures the current thinking 🙂
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.
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 😁
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.
Great points, and there's nothing stopping us from adding a
fluid
or fuzzy option as an additional control alongsidefixed
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 👍