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 3 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
99 changes: 85 additions & 14 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,18 @@ 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 [ isResizing, setIsResizing ] = useState( false );
const [ temporaryHeight, setTemporaryHeight ] = useState( null );
Expand All @@ -110,32 +115,84 @@ const SpacerEdit = ( {
const handleOnVerticalResizeStop = ( newHeight ) => {
onResizeStop();

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

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

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

if ( isFlexLayout ) {
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,6 +254,18 @@ const SpacerEdit = ( {
width: '72px',
} );
}
if ( isFlexLayout && ! flexSize ) {
setAttributes( {
style: {
Copy link
Contributor

Choose a reason for hiding this comment

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

One final nit (and not a blocker), but I just noticed that sometimes the serialized block markup contains an empty layout object within style. We can use cleanEmptyObject to remove these, e.g. like in the Cover block here:

gutenberg/packages/block-library/src/cover/transforms.js

Line 222 in 7fd28e4

style: cleanEmptyObject( {

It doesn't cause any block validation issues to retain the empty objects in the block markup, though, so I really don't think it's a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funnily enough, running the object passed to setAttributes through cleanEmptyObject results in the layout attributes not being unset at all, I'm not sure why. I might leave it as is and avoid going down a rabbit hole with this 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

I might leave it as is and avoid going down a rabbit hole with this 😅

Good thinking! I'm in support of the idea of merging this now that it's in a good and working state, and we can look at further tweaks in isolation 👍

...blockStyle,
layout: {
...layout,
flexSize: width || '72px',
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 need to account for height and/or the spacing presets values? When dragging into a Stack variation, it looks like it defaults to the 72px value:

2023-04-12 16 48 11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, well spotted! I've fixed the logic in the effect now and the problem with fill state should be addressed.

selfStretch: 'fixed',
},
},
} );
}
}, [] );

return (
Expand All @@ -211,13 +280,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