Try adding a grid layout type #49018
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 |
|---|---|---|
|
|
@@ -87,3 +87,22 @@ function allow_filter_in_styles( $allow_css, $css_test_string ) { | |
| } | ||
|
|
||
| add_filter( 'safecss_filter_attr_allow_css', 'allow_filter_in_styles', 10, 2 ); | ||
|
|
||
| /** | ||
| * Mark CSS safe if it contains grid functions | ||
| * | ||
| * This function should not be backported to core. | ||
| * | ||
| * @param bool $allow_css Whether the CSS is allowed. | ||
| * @param string $css_test_string The CSS to test. | ||
| */ | ||
| function allow_grid_functions_in_styles( $allow_css, $css_test_string ) { | ||
| if ( preg_match( | ||
| '/^grid-template-columns:\s*repeat\([0-9,a-z-\s\(\)]*\)$/', | ||
|
There was a problem hiding this comment. This looks good to me for now, but just wondering what the fix in core will be for it? I suppose we don't need to figure that out right now, but I was curious if it'll just be a matter of adding There was a problem hiding this comment. Judging by the previous update it should be pretty straightforward. |
||
| $css_test_string | ||
| ) ) { | ||
| return true; | ||
| } | ||
| return $allow_css; | ||
| } | ||
| add_filter( 'safecss_filter_attr_allow_css', 'allow_grid_functions_in_styles', 10, 2 ); | ||
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 |
|---|---|---|
|
|
@@ -352,6 +352,28 @@ | |
| } | ||
| } | ||
| ] | ||
| }, | ||
| "grid": { | ||
| "name": "grid", | ||
| "slug": "grid", | ||
| "className": "is-layout-grid", | ||
| "displayMode": "grid", | ||
| "baseStyles": [ | ||
| { | ||
| "selector": " > *", | ||
| "rules": { | ||
|
There was a problem hiding this comment. It seems there are more nuances about There was a problem hiding this comment. Ah, I added that margin override due to this comment pointing out that default block margins can interfere with the grid gap. It's the same we have for flex blocks; do you think this might be more of an issue with grid than with flex layouts? There's also a whole discussion around margins set in global styles being overridden by layout rules, that we're trying to address in #47858. There was a problem hiding this comment.
Probably the same :). Thanks for linking the discussions/issues. |
||
| "margin": "0" | ||
| } | ||
| } | ||
| ], | ||
| "spacingStyles": [ | ||
| { | ||
| "selector": "", | ||
| "rules": { | ||
| "gap": null | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }, | ||
|
|
||
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 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,172 @@ | ||||||||||||||||
| /** | ||||||||||||||||
| * WordPress dependencies | ||||||||||||||||
| */ | ||||||||||||||||
| import { __ } from '@wordpress/i18n'; | ||||||||||||||||
|
|
||||||||||||||||
| import { | ||||||||||||||||
| BaseControl, | ||||||||||||||||
| Flex, | ||||||||||||||||
| FlexItem, | ||||||||||||||||
| RangeControl, | ||||||||||||||||
| __experimentalUnitControl as UnitControl, | ||||||||||||||||
| __experimentalParseQuantityAndUnitFromRawValue as parseQuantityAndUnitFromRawValue, | ||||||||||||||||
| } from '@wordpress/components'; | ||||||||||||||||
|
|
||||||||||||||||
| /** | ||||||||||||||||
| * Internal dependencies | ||||||||||||||||
| */ | ||||||||||||||||
| import { appendSelectors, getBlockGapCSS } from './utils'; | ||||||||||||||||
| import { getGapCSSValue } from '../hooks/gap'; | ||||||||||||||||
| import { shouldSkipSerialization } from '../hooks/utils'; | ||||||||||||||||
|
|
||||||||||||||||
| const RANGE_CONTROL_MAX_VALUES = { | ||||||||||||||||
| px: 600, | ||||||||||||||||
| '%': 100, | ||||||||||||||||
| vw: 100, | ||||||||||||||||
| vh: 100, | ||||||||||||||||
| em: 38, | ||||||||||||||||
| rem: 38, | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| export default { | ||||||||||||||||
| name: 'grid', | ||||||||||||||||
| label: __( 'Grid' ), | ||||||||||||||||
| inspectorControls: function GridLayoutInspectorControls( { | ||||||||||||||||
| layout = {}, | ||||||||||||||||
| onChange, | ||||||||||||||||
| } ) { | ||||||||||||||||
| return ( | ||||||||||||||||
| <GridLayoutMinimumWidthControl | ||||||||||||||||
| layout={ layout } | ||||||||||||||||
| onChange={ onChange } | ||||||||||||||||
| /> | ||||||||||||||||
| ); | ||||||||||||||||
| }, | ||||||||||||||||
| toolBarControls: function DefaultLayoutToolbarControls() { | ||||||||||||||||
| return null; | ||||||||||||||||
| }, | ||||||||||||||||
| getLayoutStyle: function getLayoutStyle( { | ||||||||||||||||
| selector, | ||||||||||||||||
| layout, | ||||||||||||||||
| style, | ||||||||||||||||
| blockName, | ||||||||||||||||
| hasBlockGapSupport, | ||||||||||||||||
| layoutDefinitions, | ||||||||||||||||
| } ) { | ||||||||||||||||
| const { minimumColumnWidth = '12rem' } = layout; | ||||||||||||||||
|
|
||||||||||||||||
| // 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 = | ||||||||||||||||
| style?.spacing?.blockGap && | ||||||||||||||||
| ! shouldSkipSerialization( blockName, 'spacing', 'blockGap' ) | ||||||||||||||||
| ? getGapCSSValue( style?.spacing?.blockGap, '0.5em' ) | ||||||||||||||||
| : undefined; | ||||||||||||||||
|
|
||||||||||||||||
| let output = ''; | ||||||||||||||||
| const rules = []; | ||||||||||||||||
|
|
||||||||||||||||
| if ( minimumColumnWidth ) { | ||||||||||||||||
| rules.push( | ||||||||||||||||
| `grid-template-columns: repeat(auto-fill, minmax(min(${ minimumColumnWidth }, 100%), 1fr))` | ||||||||||||||||
| ); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if ( rules.length ) { | ||||||||||||||||
| // Reason to disable: the extra line breaks added by prettier mess with the unit tests. | ||||||||||||||||
| // eslint-disable-next-line prettier/prettier | ||||||||||||||||
| output = `${ appendSelectors( selector ) } { ${ rules.join( | ||||||||||||||||
| '; ' | ||||||||||||||||
| ) }; }`; | ||||||||||||||||
|
Comment on lines
+76
to
+80
There was a problem hiding this comment. Another option (totally feel free to ignore this!), if we don't want to disable prettier on this line could be to perform the output over two lines. So, something like:
Suggested change
There was a problem hiding this comment. I didn't want to dig too deeply into it in this PR, but it feels a bit absurd to have prettier formatting multi-line strings at all, because sometimes the spacing is there for a specific reason. Something to look at later perhaps? There was a problem hiding this comment.
Totally, and not a blocker at all. This was just in case you didn't want the extra disable in there. I doubt we can really configure prettier here, though, as far as I know there's not a lot of configurability. |
||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // Output blockGap styles based on rules contained in layout definitions in theme.json. | ||||||||||||||||
| if ( hasBlockGapSupport && blockGapValue ) { | ||||||||||||||||
| output += getBlockGapCSS( | ||||||||||||||||
| selector, | ||||||||||||||||
| layoutDefinitions, | ||||||||||||||||
| 'grid', | ||||||||||||||||
| blockGapValue | ||||||||||||||||
| ); | ||||||||||||||||
| } | ||||||||||||||||
| return output; | ||||||||||||||||
| }, | ||||||||||||||||
| getOrientation() { | ||||||||||||||||
| return 'horizontal'; | ||||||||||||||||
| }, | ||||||||||||||||
| getAlignments() { | ||||||||||||||||
| return []; | ||||||||||||||||
| }, | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| // Enables setting minimum width of grid items. | ||||||||||||||||
| function GridLayoutMinimumWidthControl( { layout, onChange } ) { | ||||||||||||||||
| const { minimumColumnWidth: value = '12rem' } = layout; | ||||||||||||||||
| const [ quantity, unit ] = parseQuantityAndUnitFromRawValue( value ); | ||||||||||||||||
|
|
||||||||||||||||
| const handleSliderChange = ( next ) => { | ||||||||||||||||
| onChange( { | ||||||||||||||||
| ...layout, | ||||||||||||||||
| minimumColumnWidth: [ next, unit ].join( '' ), | ||||||||||||||||
| } ); | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| // Mostly copied from HeightControl. | ||||||||||||||||
| const handleUnitChange = ( newUnit ) => { | ||||||||||||||||
| // Attempt to smooth over differences between currentUnit and newUnit. | ||||||||||||||||
| // This should slightly improve the experience of switching between unit types. | ||||||||||||||||
| let newValue; | ||||||||||||||||
|
|
||||||||||||||||
| if ( [ 'em', 'rem' ].includes( newUnit ) && unit === 'px' ) { | ||||||||||||||||
| // Convert pixel value to an approximate of the new unit, assuming a root size of 16px. | ||||||||||||||||
| newValue = ( quantity / 16 ).toFixed( 2 ) + newUnit; | ||||||||||||||||
| } else if ( [ 'em', 'rem' ].includes( unit ) && newUnit === 'px' ) { | ||||||||||||||||
| // Convert to pixel value assuming a root size of 16px. | ||||||||||||||||
| newValue = Math.round( quantity * 16 ) + newUnit; | ||||||||||||||||
| } else if ( | ||||||||||||||||
| [ 'vh', 'vw', '%' ].includes( newUnit ) && | ||||||||||||||||
| quantity > 100 | ||||||||||||||||
| ) { | ||||||||||||||||
| // When converting to `vh`, `vw`, or `%` units, cap the new value at 100. | ||||||||||||||||
| newValue = 100 + newUnit; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| onChange( { | ||||||||||||||||
| ...layout, | ||||||||||||||||
| minimumColumnWidth: newValue, | ||||||||||||||||
| } ); | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| return ( | ||||||||||||||||
| <fieldset> | ||||||||||||||||
| <BaseControl.VisualLabel as="legend"> | ||||||||||||||||
| { __( 'Minimum column width' ) } | ||||||||||||||||
| </BaseControl.VisualLabel> | ||||||||||||||||
| <Flex gap={ 4 }> | ||||||||||||||||
| <FlexItem isBlock> | ||||||||||||||||
| <UnitControl | ||||||||||||||||
| size={ '__unstable-large' } | ||||||||||||||||
| onChange={ ( newValue ) => { | ||||||||||||||||
| onChange( { | ||||||||||||||||
| ...layout, | ||||||||||||||||
| minimumColumnWidth: newValue, | ||||||||||||||||
| } ); | ||||||||||||||||
| } } | ||||||||||||||||
| onUnitChange={ handleUnitChange } | ||||||||||||||||
| value={ value } | ||||||||||||||||
| min={ 0 } | ||||||||||||||||
| /> | ||||||||||||||||
| </FlexItem> | ||||||||||||||||
| <FlexItem isBlock> | ||||||||||||||||
| <RangeControl | ||||||||||||||||
andrewserong marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
| onChange={ handleSliderChange } | ||||||||||||||||
| value={ quantity } | ||||||||||||||||
| min={ 0 } | ||||||||||||||||
| max={ RANGE_CONTROL_MAX_VALUES[ unit ] || 600 } | ||||||||||||||||
| withInputField={ false } | ||||||||||||||||
| /> | ||||||||||||||||
| </FlexItem> | ||||||||||||||||
| </Flex> | ||||||||||||||||
| </fieldset> | ||||||||||||||||
| ); | ||||||||||||||||
| } | ||||||||||||||||
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 |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import grid from '../grid'; | ||
|
|
||
| describe( 'getLayoutStyle', () => { | ||
| it( 'should return a single `grid-template-columns` property if no non-default params are provided', () => { | ||
| const expected = `.editor-styles-wrapper .my-container { grid-template-columns: repeat(auto-fill, minmax(min(12rem, 100%), 1fr)); }`; | ||
|
|
||
| const result = grid.getLayoutStyle( { | ||
| selector: '.my-container', | ||
| layout: {}, | ||
| style: {}, | ||
| blockName: 'test-block', | ||
| hasBlockGapSupport: false, | ||
| layoutDefinitions: undefined, | ||
| } ); | ||
|
|
||
| expect( result ).toBe( expected ); | ||
| } ); | ||
| } ); |
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
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.
Just a future proofing question: if
minimumColumnWidthisn't set, this assumes a default of12rem. If we want to enable the control over individual columns again in follow-ups, how will we tell that we shouldn't be outputting12rem?By which I mean, what are the trade-offs between using
12remas an inferred value, versus explicitly defining12remwithin the Grid variation? For example, what if other blocks wished to use a different default minimum column width?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.
It would be pretty easy to introduce something like
isResponsiveagain, or any other property that indicates that the grid type is not based on column width (which would always be the default). We'll need a check before outputting the value because the whole template-columns declaration will be different for a static grid, and even if aminimumColumnWidthis set (e.g. if we don't reset it on switching to another grid type) we will only use the value if the grid is of the responsive type (or whatever we want to call it).I chose 12rem arbitrarily, it could be any other value but we do need to have some default value here or the
auto-fillwon't work.If other blocks want to use a different value they can set it when they define the layout type, e.g.
layout: { type: 'grid', minimumColumnWidth: '9rem' }.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.
So (just to make sure I'm following), we're essentially saying, a grid layout with absolutely no other values will default to
12remminimum column width. For future changes, we're adding additional attributes that will be set, so it'll be easy to determine when to switch off the current logic. And for changing the12remdefault on a block-by-block basis, we provide the default type type value as you mention.Sounds solid to me! 👍
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.
Yeah, the idea is to have a sensible default so you can just add a grid and it'll look ok for the majority of cases? Then progressively add configurability.