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 child fixed size should not be fixed by default and should always have a value set #46139

Merged
merged 5 commits into from
Dec 4, 2022
Merged

Layout child fixed size should not be fixed by default and should always have a value set #46139

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
477908f
Change behaviour of fixed child size to not be fixed by default
tellthemachines Nov 29, 2022
07de731
Fix boo-boo
tellthemachines Nov 29, 2022
40d0ac2
Remove unshrink control
tellthemachines Nov 29, 2022
2ec5ac7
Determine fixed size automatically
tellthemachines Nov 30, 2022
c9e50f7
Revert "Determine fixed size automatically"
tellthemachines Dec 4, 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
5 changes: 2 additions & 3 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,8 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
$child_layout_styles[] = array(
'selector' => ".$container_content_class",
'declarations' => array(
'flex-shrink' => '0',
'flex-basis' => $block['attrs']['style']['layout']['flexSize'],
'box-sizing' => 'border-box',
'flex-basis' => $block['attrs']['style']['layout']['flexSize'],
'box-sizing' => 'border-box',
),
);
} elseif ( 'fill' === $block['attrs']['style']['layout']['selfStretch'] ) {
Expand Down
16 changes: 14 additions & 2 deletions packages/block-editor/src/hooks/child-layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import { __unstableUseBlockElement as useBlockElement } from '../components/block-list/use-block-props/use-block-refs';
import useSetting from '../components/use-setting';

function helpText( selfStretch ) {
Expand All @@ -28,13 +29,15 @@ function helpText( selfStretch ) {
* Inspector controls containing the child layout related configuration.
*
* @param {Object} props Block props.
* @param {string} props.clientId Block client id.
* @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( {
clientId,
attributes,
setAttributes,
__unstableParentLayout: parentLayout,
Expand All @@ -43,6 +46,13 @@ export function ChildLayoutEdit( {
const { layout: childLayout = {} } = style;
const { selfStretch, flexSize } = childLayout;

const blockElement = useBlockElement( clientId );
const { orientation = 'horizontal' } = parentLayout;
const blockSize =
orientation === 'horizontal'
? `${ blockElement?.offsetWidth }px`
: `${ blockElement?.offsetHeight }px`;

return (
<>
<ToggleGroupControl
Expand All @@ -51,7 +61,9 @@ export function ChildLayoutEdit( {
value={ selfStretch || 'fit' }
help={ helpText( selfStretch ) }
onChange={ ( value ) => {
const newFlexSize = value !== 'fixed' ? null : flexSize;
const defaultFlexSize = flexSize || blockSize;
const newFlexSize =
value !== 'fixed' ? null : defaultFlexSize;
setAttributes( {
style: {
...style,
Expand Down Expand Up @@ -96,7 +108,7 @@ export function ChildLayoutEdit( {
},
} );
} }
value={ flexSize }
value={ flexSize || blockSize }
Copy link
Contributor

@andrewserong andrewserong Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Using the value as a fallback in all cases appears to cause an odd behaviour when editing / removing the fixed value altogether, because it winds up getting a closer to fresh value for the width. Here's a screengrab (initially set to 200px, then after deleting the value and clicking out of the field, it sets it to 95px:

2022-11-30 13 00 34

Would it work to use blockSize within the onChange handler when switching between fit and fixed instead? Edit: ah wait, I see you're already doing that. I guess the question is then, do we need to include the fallback on value?

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 I understand the question. If we remove the fallback from value the field won't be pre-populated, which I think is a requirement?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove the fallback from value the field won't be pre-populated, which I think is a requirement?

Ah, good point, my apologies, I missed that detail of the requirement that James mentioned. I'd been thinking of the Fit -> Fixed selection and not the saved empty Fixed state problem 🤔

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, always worth asking!

Copy link
Contributor

Choose a reason for hiding this comment

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

Just thought I'd hack around a little bit. The only other way I could come up with addressing that requirement while preventing the 95px issue from showing up, would be to add a bit of logic so that when the block is initially selected, it displays as if fit were selected if fixed is selected but flexSize is empty:

	const [ showFit, setShowFit ] = useState();

	useEffect( () => {
		if ( selfStretch === 'fixed' && ! flexSize ) {
			setShowFit( true );
		}
	}, [] );

Then, in the ToggleGroupControl change handler we'd call setShowFit( false ) whenever the value changes. And the bits where we're referring to selfStretch would become the slightly awkward:

				value={ showFit ? 'fit' : selfStretch || 'fit' }
				help={ helpText( showFit ? 'fit' : selfStretch ) }

With that in place, I think we could then remove the blockSize fallback in value={ flexSize || blockSize }, but overall it would be slightly different behaviour than what was asked for.

Feel free to ignore all that, though, I was mostly curious to better understand how it's working! 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that hide the input altogether when it has no value? I'm not sure we want that; the idea is that if 'fixed' is selected the input should be pre-populated with the current width/height of the block, so we have to pass blockSize as the value unless there's an existing flexSize.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't that hide the input altogether when it has no value?

Yes, it would hide it when initially selected if it doesn't have a flexSize value, and would default to display 'fit' instead. I'm not really advocating for that, but it was the only behaviour I could figure out that avoided accidentally displaying the wrong width value when a user clears out the Fixed input field and then tabs away.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thought: the issue of the wrong width value only appears when you tab away from the cleared input field, and is correct again if you select another block and then click to re-select the block you were initially working on. Personally, I'd be fine with the idea of merging this PR as-is, and us continue to fix/tweak in follow-ups if need be.

/>
) }
</>
Expand Down
1 change: 0 additions & 1 deletion packages/block-editor/src/hooks/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,6 @@ export const withChildLayoutStyles = createHigherOrderComponent(

if ( selfStretch === 'fixed' && flexSize ) {
css += `${ selector } {
flex-shrink: 0;
flex-basis: ${ flexSize };
box-sizing: border-box;
}`;
Expand Down