Skip to content

Commit

Permalink
Grid layout: Use numbers instead of strings for layout attributes (Wo…
Browse files Browse the repository at this point in the history 
…rdPress#63112)

* Grid layout: Use numbers instead of strings for layout attributes

Ensure columnCount, rowCount, columnStart, rowStart, columnSpan, and
rowSpan are always serialized into block attributes as a number, not
a string. Previously we were inconsistent.

* I am dumb

* Update migration comment

* Unset columnCount instead of setting it to null

* Unset minimumColumnWidth instead of setting it to an empty string

* Show slider at 0 if there is no columnCount set

* Handle empty strings and invalid numbers in migration
  • Loading branch information
@noisysocks @carstingaxion
noisysocks authored and carstingaxion committed Jul 18, 2024
1 parent ec5a26b commit d0193f5
Show file tree
Hide file tree
Showing 5 changed files with 151 additions and 68 deletions.
47 changes: 28 additions & 19 deletions packages/block-editor/src/components/child-layout-control/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,16 +193,15 @@ function GridControls( {
panelId,
} ) {
const { columnStart, rowStart, columnSpan, rowSpan } = childLayout;
const { columnCount } = parentLayout ?? {};
const gridColumnNumber = parseInt( columnCount, 10 ) || 3;
const { columnCount = 3, rowCount } = parentLayout ?? {};
const rootClientId = useSelect( ( select ) =>
select( blockEditorStore ).getBlockRootClientId( panelId )
);
const { moveBlocksToPosition, __unstableMarkNextChangeAsNotPersistent } =
useDispatch( blockEditorStore );
const getNumberOfBlocksBeforeCell = useGetNumberOfBlocksBeforeCell(
rootClientId,
gridColumnNumber
columnCount
);
const hasStartValue = () => !! columnStart || !! rowStart;
const hasSpanValue = () => !! columnSpan || !! rowSpan;
Expand Down Expand Up @@ -234,29 +233,35 @@ function GridControls( {
label={ __( 'Column span' ) }
type="number"
onChange={ ( value ) => {
// Don't allow unsetting.
const newColumnSpan =
value === '' ? 1 : parseInt( value, 10 );
onChange( {
columnStart,
rowStart,
rowSpan,
columnSpan: value,
columnSpan: newColumnSpan,
} );
} }
value={ columnSpan }
value={ columnSpan ?? 1 }
min={ 1 }
/>
<InputControl
size="__unstable-large"
label={ __( 'Row span' ) }
type="number"
onChange={ ( value ) => {
// Don't allow unsetting.
const newRowSpan =
value === '' ? 1 : parseInt( value, 10 );
onChange( {
columnStart,
rowStart,
columnSpan,
rowSpan: value,
rowSpan: newRowSpan,
} );
} }
value={ rowSpan }
value={ rowSpan ?? 1 }
min={ 1 }
/>
</HStack>
Expand All @@ -278,8 +283,11 @@ function GridControls( {
label={ __( 'Column' ) }
type="number"
onChange={ ( value ) => {
// Don't allow unsetting.
const newColumnStart =
value === '' ? 1 : parseInt( value, 10 );
onChange( {
columnStart: value,
columnStart: newColumnStart,
rowStart,
columnSpan,
rowSpan,
Expand All @@ -290,16 +298,16 @@ function GridControls( {
rootClientId,
rootClientId,
getNumberOfBlocksBeforeCell(
value,
newColumnStart,
rowStart
)
);
} }
value={ columnStart }
value={ columnStart ?? 1 }
min={ 1 }
max={
gridColumnNumber
? gridColumnNumber - ( columnSpan ?? 1 ) + 1
columnCount
? columnCount - ( columnSpan ?? 1 ) + 1
: undefined
}
/>
Expand All @@ -310,9 +318,12 @@ function GridControls( {
label={ __( 'Row' ) }
type="number"
onChange={ ( value ) => {
// Don't allow unsetting.
const newRowStart =
value === '' ? 1 : parseInt( value, 10 );
onChange( {
columnStart,
rowStart: value,
rowStart: newRowStart,
columnSpan,
rowSpan,
} );
Expand All @@ -323,17 +334,15 @@ function GridControls( {
rootClientId,
getNumberOfBlocksBeforeCell(
columnStart,
value
newRowStart
)
);
} }
value={ rowStart }
value={ rowStart ?? 1 }
min={ 1 }
max={
parentLayout?.rowCount
? parentLayout.rowCount -
( rowSpan ?? 1 ) +
1
rowCount
? rowCount - ( rowSpan ?? 1 ) + 1
: undefined
}
/>
Expand Down
30 changes: 7 additions & 23 deletions packages/block-editor/src/components/grid/grid-item-movers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,9 @@ export function GridItemMovers( {
const columnCount = parentLayout?.columnCount;
const rowCount = parentLayout?.rowCount;

const columnCountNumber = parseInt( columnCount, 10 );
const rowStartNumber = parseInt( rowStart, 10 );
const columnStartNumber = parseInt( columnStart, 10 );

const getNumberOfBlocksBeforeCell = useGetNumberOfBlocksBeforeCell(
gridClientId,
columnCountNumber
columnCount
);

return (
Expand All @@ -56,10 +52,7 @@ export function GridItemMovers( {
[ blockClientId ],
gridClientId,
gridClientId,
getNumberOfBlocksBeforeCell(
columnStartNumber,
rowStartNumber - 1
)
getNumberOfBlocksBeforeCell( columnStart, rowStart - 1 )
);
} }
/>
Expand All @@ -76,10 +69,7 @@ export function GridItemMovers( {
[ blockClientId ],
gridClientId,
gridClientId,
getNumberOfBlocksBeforeCell(
columnStartNumber,
rowStartNumber + 1
)
getNumberOfBlocksBeforeCell( columnStart, rowStart + 1 )
);
} }
/>
Expand All @@ -89,17 +79,14 @@ export function GridItemMovers( {
disabled={ columnStart <= 1 }
onClick={ () => {
onChange( {
columnStart: columnStartNumber - 1,
columnStart: columnStart - 1,
} );
__unstableMarkNextChangeAsNotPersistent();
moveBlocksToPosition(
[ blockClientId ],
gridClientId,
gridClientId,
getNumberOfBlocksBeforeCell(
columnStartNumber - 1,
rowStartNumber
)
getNumberOfBlocksBeforeCell( columnStart - 1, rowStart )
);
} }
/>
Expand All @@ -109,17 +96,14 @@ export function GridItemMovers( {
disabled={ columnCount && columnEnd >= columnCount }
onClick={ () => {
onChange( {
columnStart: columnStartNumber + 1,
columnStart: columnStart + 1,
} );
__unstableMarkNextChangeAsNotPersistent();
moveBlocksToPosition(
[ blockClientId ],
gridClientId,
gridClientId,
getNumberOfBlocksBeforeCell(
columnStartNumber + 1,
rowStartNumber
)
getNumberOfBlocksBeforeCell( columnStart + 1, rowStart )
);
} }
/>
Expand Down
28 changes: 18 additions & 10 deletions packages/block-editor/src/hooks/layout-child.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,23 @@ function useBlockPropsChildLayoutStyles( { style } ) {
const id = useInstanceId( useBlockPropsChildLayoutStyles );
const selector = `.wp-container-content-${ id }`;

// Check that the grid layout attributes are of the correct type, so that we don't accidentally
// write code that stores a string attribute instead of a number.
if ( process.env.NODE_ENV === 'development' ) {
if ( columnStart && typeof columnStart !== 'number' ) {
throw new Error( 'columnStart must be a number' );
}
if ( rowStart && typeof rowStart !== 'number' ) {
throw new Error( 'rowStart must be a number' );
}
if ( columnSpan && typeof columnSpan !== 'number' ) {
throw new Error( 'columnSpan must be a number' );
}
if ( rowSpan && typeof rowSpan !== 'number' ) {
throw new Error( 'rowSpan must be a number' );
}
}

let css = '';
if ( shouldRenderChildLayoutStyles ) {
if ( selfStretch === 'fixed' && flexSize ) {
Expand Down Expand Up @@ -81,16 +98,6 @@ function useBlockPropsChildLayoutStyles( { style } ) {
( columnSpan || columnStart ) &&
( minimumColumnWidth || ! columnCount )
) {
// Check if columnSpan and columnStart are numbers so Math.max doesn't break.
const columnSpanNumber = columnSpan ? parseInt( columnSpan ) : null;
const columnStartNumber = columnStart
? parseInt( columnStart )
: null;
const highestNumber = Math.max(
columnSpanNumber,
columnStartNumber
);

let parentColumnValue = parseFloat( minimumColumnWidth );
/**
* 12rem is the default minimumColumnWidth value.
Expand All @@ -112,6 +119,7 @@ function useBlockPropsChildLayoutStyles( { style } ) {
parentColumnUnit = 'rem';
}

const highestNumber = Math.max( columnSpan, columnStart );
const defaultGapValue = parentColumnUnit === 'px' ? 24 : 1.5;
const containerQueryValue =
highestNumber * parentColumnValue +
Expand Down
76 changes: 60 additions & 16 deletions packages/block-editor/src/layouts/grid.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,23 @@ export default {
rowCount = null,
} = layout;

// Check that the grid layout attributes are of the correct type, so that we don't accidentally
// write code that stores a string attribute instead of a number.
if ( process.env.NODE_ENV === 'development' ) {
if (
minimumColumnWidth &&
typeof minimumColumnWidth !== 'string'
) {
throw new Error( 'minimumColumnWidth must be a string' );
}
if ( columnCount && typeof columnCount !== 'number' ) {
throw new Error( 'columnCount must be a number' );
}
if ( rowCount && typeof rowCount !== 'number' ) {
throw new Error( 'rowCount must be a number' );
}
}

// If a block's block.json skips serialization for spacing or spacing.blockGap,
// don't apply the user-defined value to the styles.
const blockGapValue =
Expand Down Expand Up @@ -241,7 +258,8 @@ function GridLayoutMinimumWidthControl( { layout, onChange } ) {
onChange={ ( newValue ) => {
onChange( {
...layout,
minimumColumnWidth: newValue,
minimumColumnWidth:
newValue === '' ? undefined : newValue,
} );
} }
onUnitChange={ handleUnitChange }
Expand Down Expand Up @@ -274,7 +292,15 @@ function GridLayoutColumnsAndRowsControl( {
onChange,
allowSizingOnChildren,
} ) {
const { columnCount = 3, rowCount, isManualPlacement } = layout;
// If the grid interactivity experiment is enabled, allow unsetting the column count.
const defaultColumnCount = window.__experimentalEnableGridInteractivity
? undefined
: 3;
const {
columnCount = defaultColumnCount,
rowCount,
isManualPlacement,
} = layout;

return (
<>
Expand All @@ -290,18 +316,31 @@ function GridLayoutColumnsAndRowsControl( {
<NumberControl
size="__unstable-large"
onChange={ ( value ) => {
/**
* If the input is cleared, avoid switching
* back to "Auto" by setting a value of "1".
*/
const validValue = value !== '' ? value : '1';
onChange( {
...layout,
columnCount:
window.__experimentalEnableGridInteractivity
? parseInt( value, 10 ) || null
: parseInt( validValue, 10 ),
} );
if (
window.__experimentalEnableGridInteractivity
) {
// Allow unsetting the column count when in auto mode.
const defaultNewColumnCount =
isManualPlacement ? 1 : undefined;
const newColumnCount =
value === ''
? defaultNewColumnCount
: parseInt( value, 10 );
onChange( {
...layout,
columnCount: newColumnCount,
} );
} else {
// Don't allow unsetting the column count.
const newColumnCount =
value === ''
? 1
: parseInt( value, 10 );
onChange( {
...layout,
columnCount: newColumnCount,
} );
}
} }
value={ columnCount }
min={ 0 }
Expand All @@ -320,9 +359,14 @@ function GridLayoutColumnsAndRowsControl( {
<NumberControl
size="__unstable-large"
onChange={ ( value ) => {
// Don't allow unsetting the row count.
const newRowCount =
value === ''
? 1
: parseInt( value, 10 );
onChange( {
...layout,
rowCount: parseInt( value, 10 ),
rowCount: newRowCount,
} );
} }
value={ rowCount }
Expand All @@ -331,7 +375,7 @@ function GridLayoutColumnsAndRowsControl( {
/>
) : (
<RangeControl
value={ parseInt( columnCount, 10 ) } // RangeControl can't deal with strings.
value={ columnCount ?? 0 }
onChange={ ( value ) =>
onChange( {
...layout,
Expand Down
Loading

0 comments on commit d0193f5

Please sign in to comment.