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

Adapt flex child controls for Spacer. #49362

Merged
merged 9 commits into from
Apr 19, 2023
Merged

Adapt flex child controls for Spacer. #49362

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b833bff
Adapt flex child controls for Spacer.
tellthemachines Mar 27, 2023
c709721
Resize from fixed input field
tellthemachines Mar 28, 2023
e3d1364
Make sure initial state is "Fixed"
tellthemachines Apr 12, 2023
7d751d2
Fix buggy fill state
tellthemachines Apr 13, 2023
de0f39b
Fix size discrepancies while dragging
tellthemachines Apr 14, 2023
adcc4f8
re-add dependencies
tellthemachines Apr 14, 2023
f64baf7
Change presets to custom values in flex containers
tellthemachines Apr 18, 2023
8f1c8f9
actually remove unnecessary attributes
tellthemachines Apr 18, 2023
1a3623c
Fix block validation error and remove redundant inline styles
tellthemachines Apr 18, 2023
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
135 changes: 120 additions & 15 deletions packages/block-library/src/spacer/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,20 @@ const SpacerEdit = ( {
} );
const { orientation } = context;
const { orientation: parentOrientation, type } = parentLayout || {};
// Check if the spacer is inside a flex container.
const isFlexLayout = type === 'flex';
// If the spacer is inside a flex container, it should either inherit the orientation
// of the parent or use the flex default orientation.
const inheritedOrientation =
! parentOrientation && type === 'flex'
! parentOrientation && isFlexLayout
? 'horizontal'
: parentOrientation || orientation;
const { height, width } = attributes;
const { height, width, style: blockStyle = {} } = attributes;

const { layout = {} } = blockStyle;
const { selfStretch, flexSize } = layout;

const hasFlexSize = !! flexSize;

const [ isResizing, setIsResizing ] = useState( false );
const [ temporaryHeight, setTemporaryHeight ] = useState( null );
Expand All @@ -110,32 +117,84 @@ const SpacerEdit = ( {
const handleOnVerticalResizeStop = ( newHeight ) => {
onResizeStop();

if ( isFlexLayout || hasFlexSize ) {
setAttributes( {
style: {
...blockStyle,
layout: {
...layout,
flexSize: newHeight,
selfStretch: 'fixed',
},
},
} );
}

setAttributes( { height: newHeight } );
setTemporaryHeight( null );
};

const handleOnHorizontalResizeStop = ( newWidth ) => {
onResizeStop();

if ( isFlexLayout || hasFlexSize ) {
setAttributes( {
style: {
...blockStyle,
layout: {
...layout,
flexSize: newWidth,
selfStretch: 'fixed',
},
},
} );
}

setAttributes( { width: newWidth } );
setTemporaryWidth( null );
};

const getHeightForVerticalBlocks = () => {
if ( isFlexLayout && selfStretch === 'fit' ) {
return undefined;
} else if ( isFlexLayout && flexSize ) {
return temporaryHeight || flexSize;
}
return temporaryHeight || getSpacingPresetCssVar( height ) || undefined;
};

const getWidthForHorizontalBlocks = () => {
if ( isFlexLayout && selfStretch === 'fit' ) {
return undefined;
} else if ( isFlexLayout && flexSize ) {
return temporaryWidth || flexSize;
}
return temporaryWidth || getSpacingPresetCssVar( width ) || undefined;
};

const sizeConditionalOnOrientation =
inheritedOrientation === 'horizontal'
? getWidthForHorizontalBlocks()
: getHeightForVerticalBlocks();

const style = {
height:
inheritedOrientation === 'horizontal'
? 24
: temporaryHeight ||
getSpacingPresetCssVar( height ) ||
undefined,
: getHeightForVerticalBlocks(),
width:
inheritedOrientation === 'horizontal'
? temporaryWidth || getSpacingPresetCssVar( width ) || undefined
? getWidthForHorizontalBlocks()
: undefined,
// In vertical flex containers, the spacer shrinks to nothing without a minimum width.
minWidth:
inheritedOrientation === 'vertical' && type === 'flex'
inheritedOrientation === 'vertical' && isFlexLayout
? 48
: undefined,
// Add flex-basis so temporary sizes are respected.
flexBasis: isFlexLayout ? sizeConditionalOnOrientation : undefined,
// Remove flex-grow when resizing.
flexGrow: isFlexLayout && isResizing ? 0 : undefined,
};

const resizableBoxWithOrientation = ( blockOrientation ) => {
Expand Down Expand Up @@ -197,7 +256,51 @@ const SpacerEdit = ( {
width: '72px',
} );
}
}, [] );
if (
isFlexLayout &&
selfStretch !== 'fill' &&
selfStretch !== 'fit' &&
! flexSize
) {
const newSize =
inheritedOrientation === 'horizontal'
? getSpacingPresetCssVar( width ) || '72px'
: getSpacingPresetCssVar( height ) || '100px';
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be working a bit better now, but it looks like there's still a couple of issues when dragging a Spacer block in and out of a flex container. Here's a quick gif:

2023-04-13 15 18 19

I'm not too sure what's happening, unfortunately I've found it a little hard to figure out, but my observations so far are:

  • The conversion from the spacing preset to the px value doesn't appear to populate the Fixed input field
  • When moving into the flex container block and out again, somewhere along the way, it seems that the size values become stale. So some of the time the value appears to be inconsistent (like it's remembering the one set in the flex state or vice-versa)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The conversion from the spacing preset to the px value doesn't appear to populate the Fixed input field

It's not converting, so the input doesn't understand the value of the preset. Perhaps we should make it possible to set preset sizes in the child layout controls; I'm sure it would be useful outside of this scenario?

When moving into the flex container block and out again, somewhere along the way, it seems that the size values become stale

Oooh well spotted! We're alternately setting height and flexSize and not changing either of them when setting the other, so the block attributes end up something like: {"height":"var:preset|spacing|40","style":{"layout":{"flexSize":"130px","selfStretch":"fixed"}}}. I'll look into it!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not converting

Ah, I think I'm following along now. I like the idea of adding support for presets for the child layout controls, but I wonder if we'd ultimately want a different scale for size presets than spacing, given that width and height values will often need to be much bigger than spacing between things?

For now, what do you think about potentially using getCustomValueFromPreset instead of getSpacingPresetCssVar so that when dragging from outside a Row / Stack block into one of them, we switch to using the real pixel 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 wonder if we'd ultimately want a different scale for size presets than spacing, given that width and height values will often need to be much bigger than spacing between things?

Yeah, that's a great point! I'll convert to px for now.

setAttributes( {
style: {
...blockStyle,
layout: {
...layout,
flexSize: newSize,
selfStretch: 'fixed',
},
},
} );
} else if (
isFlexLayout &&
( selfStretch === 'fill' || selfStretch === 'fit' )
) {
if ( inheritedOrientation === 'horizontal' ) {
setAttributes( {
width: '0px',
} );
} else {
setAttributes( {
height: '0px',
} );
}
}
}, [
blockStyle,
flexSize,
height,
inheritedOrientation,
isFlexLayout,
layout,
selfStretch,
setAttributes,
width,
] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that the logic here will be called more frequently now? And if so, is that a safe change? From a quick skim it sounds like it's intentional for it to be called more frequently, but I wasn't sure if there's a potential state it could get in where setAttributes could result in the effect being run too often 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good point, I'm not 100% sure there won't be nefarious side effects. I mainly added the dependencies in because the linter was complaining about it 😅 and it didn't seem to do any harm, but negative perf impacts often don't show immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: I tried removing them and ended up re-adding as the effect wasn't running when a Spacer changed to "fit" or "fill" and the old width/height value was still showing up when dragging outside of the flex container. It doesn't seem to hugely increase the times the effect runs though I might easily be missing some edge case situations.

Copy link
Contributor

Choose a reason for hiding this comment

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

In practice, I don't think I've encountered any issues with this list of dependencies so far. It might just be something for us to keep an eye on if anyone runs into any issues with the spacer?


return (
<>
Expand All @@ -211,13 +314,15 @@ const SpacerEdit = ( {
>
{ resizableBoxWithOrientation( inheritedOrientation ) }
</View>
<SpacerControls
setAttributes={ setAttributes }
height={ temporaryHeight || height }
width={ temporaryWidth || width }
orientation={ inheritedOrientation }
isResizing={ isResizing }
/>
{ ! isFlexLayout && (
<SpacerControls
setAttributes={ setAttributes }
height={ temporaryHeight || height }
width={ temporaryWidth || width }
orientation={ inheritedOrientation }
isResizing={ isResizing }
/>
) }
</>
);
};
Expand Down