Adapt flex child controls for Spacer. #49362
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ import classnames from 'classnames'; | |
| */ | ||
| import { | ||
| useBlockProps, | ||
| useSetting, | ||
| getCustomValueFromPreset, | ||
| getSpacingPresetCssVar, | ||
| store as blockEditorStore, | ||
| } from '@wordpress/block-editor'; | ||
|
|
@@ -92,13 +94,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 && isFlexLayout | ||
| ? 'horizontal' | ||
| : parentOrientation || orientation; | ||
| const { height, width, style: blockStyle = {} } = attributes; | ||
|
|
||
| const { layout = {} } = blockStyle; | ||
| const { selfStretch, flexSize } = layout; | ||
|
|
||
| const spacingSizes = useSetting( 'spacing.spacingSizes' ); | ||
|
|
||
| const [ isResizing, setIsResizing ] = useState( false ); | ||
| const [ temporaryHeight, setTemporaryHeight ] = useState( null ); | ||
|
|
@@ -110,32 +119,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 | ||
| : getHeightForVerticalBlocks(), | ||
| width: | ||
| inheritedOrientation === 'horizontal' | ||
| ? getWidthForHorizontalBlocks() | ||
| : undefined, | ||
| // In vertical flex containers, the spacer shrinks to nothing without a minimum width. | ||
| minWidth: | ||
| 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 ) => { | ||
|
|
@@ -191,13 +252,93 @@ const SpacerEdit = ( { | |
| }; | ||
|
|
||
| useEffect( () => { | ||
| if ( | ||
| isFlexLayout && | ||
| selfStretch !== 'fill' && | ||
| selfStretch !== 'fit' && | ||
| ! flexSize | ||
| ) { | ||
| if ( inheritedOrientation === 'horizontal' ) { | ||
| // If spacer is moving from a vertical container to a horizontal container, | ||
| // it might not have width but have height instead. | ||
| const newSize = | ||
| getCustomValueFromPreset( width, spacingSizes ) || | ||
| getCustomValueFromPreset( height, spacingSizes ) || | ||
| '100px'; | ||
| setAttributes( { | ||
| width: undefined, | ||
| style: { | ||
| ...blockStyle, | ||
| layout: { | ||
| ...layout, | ||
| flexSize: newSize, | ||
| selfStretch: 'fixed', | ||
| }, | ||
| }, | ||
| } ); | ||
| } else { | ||
| const newSize = | ||
| getCustomValueFromPreset( height, spacingSizes ) || | ||
| getCustomValueFromPreset( width, spacingSizes ) || | ||
| '100px'; | ||
| setAttributes( { | ||
| height: undefined, | ||
| style: { | ||
| ...blockStyle, | ||
| layout: { | ||
| ...layout, | ||
| flexSize: newSize, | ||
| selfStretch: 'fixed', | ||
| }, | ||
| }, | ||
| } ); | ||
| } | ||
| } else if ( | ||
| isFlexLayout && | ||
| ( selfStretch === 'fill' || selfStretch === 'fit' ) | ||
| ) { | ||
| if ( inheritedOrientation === 'horizontal' ) { | ||
| setAttributes( { | ||
| width: undefined, | ||
| } ); | ||
| } else { | ||
| setAttributes( { | ||
| height: undefined, | ||
| } ); | ||
| } | ||
| } else if ( ! isFlexLayout && ( selfStretch || flexSize ) ) { | ||
| if ( inheritedOrientation === 'horizontal' ) { | ||
| setAttributes( { | ||
| width: flexSize, | ||
| } ); | ||
| } else { | ||
| setAttributes( { | ||
| height: flexSize, | ||
| } ); | ||
| } | ||
| setAttributes( { | ||
| style: { | ||
|
There was a problem hiding this comment. One final nit (and not a blocker), but I just noticed that sometimes the serialized block markup contains an empty 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. There was a problem hiding this comment. Funnily enough, running the object passed to There was a problem hiding this comment.
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: undefined, | ||
| selfStretch: undefined, | ||
| }, | ||
| }, | ||
| } ); | ||
| } | ||
| }, [ | ||
| blockStyle, | ||
| flexSize, | ||
| height, | ||
| inheritedOrientation, | ||
| isFlexLayout, | ||
| layout, | ||
| selfStretch, | ||
| setAttributes, | ||
| spacingSizes, | ||
| width, | ||
| ] ); | ||
|
|
||
| return ( | ||
| <> | ||
|
|
@@ -211,13 +352,15 @@ const SpacerEdit = ( { | |
| > | ||
| { resizableBoxWithOrientation( inheritedOrientation ) } | ||
| </View> | ||
| { ! isFlexLayout && ( | ||
| <SpacerControls | ||
| setAttributes={ setAttributes } | ||
| height={ temporaryHeight || height } | ||
| width={ temporaryWidth || width } | ||
| orientation={ inheritedOrientation } | ||
| isResizing={ isResizing } | ||
| /> | ||
| ) } | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 totally forgot that we're of course working in a different package when making changes to the Spacer block, so this function needs to be exported now.
Shall we rename it to
getSpacingCustomValueFromPresetorgetCustomSpacingValueFromPresetsince it's being exported from the block editor package? That way if we have other functions for different kinds of presets the need to be exported in the future, we won't have a collision?That said, from a quick search I see that the native version of
index.native.jsfor the block editor package is already exporting this function as-is here: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/index.native.js#L79.So, perhaps another option would be to leave it as-is and if in the future we need a more generic
getCustomValueFromPresetto be exported that handles other kinds of presets, then we could add a parent function that internally calls the existing spacing one.What do you think?
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'd leave it as is for now, and then perhaps if/when we need to handle other types of presets, we can reimplement this function to deal with any preset, so we don't have to change existing usage (we'd probably want to move it out of the spacing utils if we did that of course).