From e74c65974b59900b67709c11951f934eb0e63a68 Mon Sep 17 00:00:00 2001 From: Jan Potoms <2109932+Janpot@users.noreply.github.com> Date: Thu, 3 Oct 2024 21:57:16 +0200 Subject: [PATCH] [grid] Fix column spacing for nested containers (#43733) --- packages/mui-material/src/Grid2/Grid2.tsx | 54 ++++++----- .../src/PigmentGrid/PigmentGrid.tsx | 46 ---------- packages/mui-system/src/Grid/Grid.tsx | 27 +++--- packages/mui-system/src/Grid/GridProps.ts | 27 +++--- packages/mui-system/src/Grid/createGrid.tsx | 7 +- .../mui-system/src/Grid/gridGenerator.test.js | 92 ++++++++++++++----- packages/mui-system/src/Grid/gridGenerator.ts | 92 +++++++------------ test/regressions/fixtures/Grid/Issue43707.js | 79 ++++++++++++++++ 8 files changed, 252 insertions(+), 172 deletions(-) create mode 100644 test/regressions/fixtures/Grid/Issue43707.js diff --git a/packages/mui-material/src/Grid2/Grid2.tsx b/packages/mui-material/src/Grid2/Grid2.tsx index dfa589fdaa1693..bd822ca4521616 100644 --- a/packages/mui-material/src/Grid2/Grid2.tsx +++ b/packages/mui-material/src/Grid2/Grid2.tsx @@ -53,24 +53,29 @@ export interface GridBaseProps { offset?: ResponsiveStyleValue; /** * @internal - * The level of the grid starts from `0` - * and increases when the grid nests inside another grid regardless of container or item. + * The level of the grid starts from `0` and increases when the grid nests + * inside another grid. Nesting is defined as a container Grid being a direct + * child of a container Grid. * * ```js - * // level 0 - * // level 1 - * // level 2 - * // level 1 + * // level 0 + * // level 1 + * // level 2 * ``` * - * Only consecutive grid is considered nesting. - * A grid container will start at `0` if there are non-Grid element above it. + * Only consecutive grid is considered nesting. A grid container will start at + * `0` if there are non-Grid container element above it. * * ```js - * // level 0 + * // level 0 *
- * // level 0 - * // level 1 + * // level 0 + * ``` + * + * ```js + * // level 0 + * + * // level 0 * ``` */ unstable_level?: number; @@ -223,24 +228,29 @@ Grid2.propTypes /* remove-proptypes */ = { ]), /** * @internal - * The level of the grid starts from `0` - * and increases when the grid nests inside another grid regardless of container or item. + * The level of the grid starts from `0` and increases when the grid nests + * inside another grid. Nesting is defined as a container Grid being a direct + * child of a container Grid. * * ```js - * // level 0 - * // level 1 - * // level 2 - * // level 1 + * // level 0 + * // level 1 + * // level 2 * ``` * - * Only consecutive grid is considered nesting. - * A grid container will start at `0` if there are non-Grid element above it. + * Only consecutive grid is considered nesting. A grid container will start at + * `0` if there are non-Grid container element above it. * * ```js - * // level 0 + * // level 0 *
- * // level 0 - * // level 1 + * // level 0 + * ``` + * + * ```js + * // level 0 + * + * // level 0 * ``` */ unstable_level: PropTypes.number, diff --git a/packages/mui-material/src/PigmentGrid/PigmentGrid.tsx b/packages/mui-material/src/PigmentGrid/PigmentGrid.tsx index 9518069c45bf1f..6d514b7064b4d5 100644 --- a/packages/mui-material/src/PigmentGrid/PigmentGrid.tsx +++ b/packages/mui-material/src/PigmentGrid/PigmentGrid.tsx @@ -55,29 +55,6 @@ export interface GridBaseProps { * Defines the offset of the grid. */ offset?: ResponsiveStyleValue | undefined; - /** - * @internal - * The level of the grid starts from `0` - * and increases when the grid nests inside another grid regardless of container or item. - * - * ```js - * // level 0 - * // level 1 - * // level 2 - * // level 1 - * ``` - * - * Only consecutive grid is considered nesting. - * A grid container will start at `0` if there are non-Grid element above it. - * - * ```js - * // level 0 - *
- * // level 0 - * // level 1 - * ``` - */ - unstable_level?: number; /** * Defines the vertical space between the type `item` components. * It overrides the value of the `spacing` prop. @@ -252,29 +229,6 @@ PigmentGrid.propTypes /* remove-proptypes */ = { PropTypes.func, PropTypes.object, ]), - /** - * @internal - * The level of the grid starts from `0` - * and increases when the grid nests inside another grid regardless of container or item. - * - * ```js - * // level 0 - * // level 1 - * // level 2 - * // level 1 - * ``` - * - * Only consecutive grid is considered nesting. - * A grid container will start at `0` if there are non-Grid element above it. - * - * ```js - * // level 0 - *
- * // level 0 - * // level 1 - * ``` - */ - unstable_level: PropTypes.number, /** * Defines the `flex-wrap` style property. * It's applied for all screen sizes. diff --git a/packages/mui-system/src/Grid/Grid.tsx b/packages/mui-system/src/Grid/Grid.tsx index f9a7ba0ee0d26a..4e708392ba88df 100644 --- a/packages/mui-system/src/Grid/Grid.tsx +++ b/packages/mui-system/src/Grid/Grid.tsx @@ -108,24 +108,29 @@ Grid.propTypes /* remove-proptypes */ = { ]), /** * @internal - * The level of the grid starts from `0` - * and increases when the grid nests inside another grid regardless of container or item. + * The level of the grid starts from `0` and increases when the grid nests + * inside another grid. Nesting is defined as a container Grid being a direct + * child of a container Grid. * * ```js - * // level 0 - * // level 1 - * // level 2 - * // level 1 + * // level 0 + * // level 1 + * // level 2 * ``` * - * Only consecutive grid is considered nesting. - * A grid container will start at `0` if there are non-Grid element above it. + * Only consecutive grid is considered nesting. A grid container will start at + * `0` if there are non-Grid container element above it. * * ```js - * // level 0 + * // level 0 *
- * // level 0 - * // level 1 + * // level 0 + * ``` + * + * ```js + * // level 0 + * + * // level 0 * ``` */ unstable_level: PropTypes.number, diff --git a/packages/mui-system/src/Grid/GridProps.ts b/packages/mui-system/src/Grid/GridProps.ts index 51f891342908ee..3886e6b39db2ec 100644 --- a/packages/mui-system/src/Grid/GridProps.ts +++ b/packages/mui-system/src/Grid/GridProps.ts @@ -49,24 +49,29 @@ export interface GridBaseProps { offset?: ResponsiveStyleValue; /** * @internal - * The level of the grid starts from `0` - * and increases when the grid nests inside another grid regardless of container or item. + * The level of the grid starts from `0` and increases when the grid nests + * inside another grid. Nesting is defined as a container Grid being a direct + * child of a container Grid. * * ```js - * // level 0 - * // level 1 - * // level 2 - * // level 1 + * // level 0 + * // level 1 + * // level 2 * ``` * - * Only consecutive grid is considered nesting. - * A grid container will start at `0` if there are non-Grid element above it. + * Only consecutive grid is considered nesting. A grid container will start at + * `0` if there are non-Grid container element above it. * * ```js - * // level 0 + * // level 0 *
- * // level 0 - * // level 1 + * // level 0 + * ``` + * + * ```js + * // level 0 + * + * // level 0 * ``` */ unstable_level?: number; diff --git a/packages/mui-system/src/Grid/createGrid.tsx b/packages/mui-system/src/Grid/createGrid.tsx index 0f43262e5ffc7a..e51923ec147a8d 100644 --- a/packages/mui-system/src/Grid/createGrid.tsx +++ b/packages/mui-system/src/Grid/createGrid.tsx @@ -169,7 +169,12 @@ export default function createGrid( {...other} > {React.Children.map(children, (child) => { - if (React.isValidElement(child) && isMuiElement(child, ['Grid'])) { + if ( + React.isValidElement(child) && + isMuiElement(child, ['Grid']) && + container && + child.props.container + ) { return React.cloneElement(child, { unstable_level: (child.props as GridProps)?.unstable_level ?? level + 1, } as GridProps); diff --git a/packages/mui-system/src/Grid/gridGenerator.test.js b/packages/mui-system/src/Grid/gridGenerator.test.js index 95222dbdf8843b..f06d54b3461def 100644 --- a/packages/mui-system/src/Grid/gridGenerator.test.js +++ b/packages/mui-system/src/Grid/gridGenerator.test.js @@ -33,14 +33,14 @@ describe('grid generator', () => { it('nested container level 1', () => { const result = generateGridStyles({ ownerState: { container: true, unstable_level: 1 } }); sinon.assert.match(result, { - gap: `var(--Grid-rowSpacingLevel1) var(--Grid-columnSpacingLevel1)`, + gap: `var(--Grid-rowSpacing) var(--Grid-columnSpacing)`, }); }); it('nested container level 2', () => { const result = generateGridStyles({ ownerState: { container: true, unstable_level: 2 } }); sinon.assert.match(result, { - gap: `var(--Grid-rowSpacingLevel2) var(--Grid-columnSpacingLevel2)`, + gap: `var(--Grid-rowSpacing) var(--Grid-columnSpacing)`, }); }); @@ -80,7 +80,7 @@ describe('grid generator', () => { '@media (min-width:600px)': { flexBasis: 'auto', flexGrow: 0, - width: `calc(100% * 6 / var(--Grid-columns) - (var(--Grid-columns) - 6) * (var(--Grid-columnSpacing) / var(--Grid-columns)))`, + width: `calc(100% * 6 / var(--Grid-parent-columns) - (var(--Grid-parent-columns) - 6) * (var(--Grid-parent-columnSpacing) / var(--Grid-parent-columns)))`, }, '@media (min-width:900px)': { flexBasis: 0, @@ -90,7 +90,7 @@ describe('grid generator', () => { '@media (min-width:1200px)': { flexBasis: 'auto', flexGrow: 0, - width: `calc(100% * 4 / var(--Grid-columns) - (var(--Grid-columns) - 4) * (var(--Grid-columnSpacing) / var(--Grid-columns)))`, + width: `calc(100% * 4 / var(--Grid-parent-columns) - (var(--Grid-parent-columns) - 4) * (var(--Grid-parent-columnSpacing) / var(--Grid-parent-columns)))`, }, '@media (min-width:1536px)': { flexBasis: 'auto', @@ -112,6 +112,9 @@ describe('grid generator', () => { }), ).to.deep.equal({ '--Grid-columns': 16, + '> *': { + '--Grid-parent-columns': 16, + }, }); }); @@ -123,14 +126,26 @@ describe('grid generator', () => { }), ).to.deep.equal({ '--Grid-columns': 6, + '> *': { + '--Grid-parent-columns': 6, + }, '@media (min-width:600px)': { '--Grid-columns': 8, + '> *': { + '--Grid-parent-columns': 8, + }, }, '@media (min-width:900px)': { '--Grid-columns': 12, + '> *': { + '--Grid-parent-columns': 12, + }, }, '@media (min-width:1200px)': { '--Grid-columns': 16, + '> *': { + '--Grid-parent-columns': 16, + }, }, }); }); @@ -145,6 +160,9 @@ describe('grid generator', () => { '--Grid-columns': 12, '@media (min-width:1200px)': { '--Grid-columns': 16, + '> *': { + '--Grid-parent-columns': 16, + }, }, }); }); @@ -159,6 +177,9 @@ describe('grid generator', () => { }), ).to.deep.equal({ '--Grid-rowSpacing': '16px', + '> *': { + '--Grid-parent-rowSpacing': '16px', + }, }); }); @@ -170,6 +191,9 @@ describe('grid generator', () => { }), ).to.deep.equal({ '--Grid-rowSpacing': '1rem', + '> *': { + '--Grid-parent-rowSpacing': '1rem', + }, }); }); @@ -181,11 +205,20 @@ describe('grid generator', () => { }), ).to.deep.equal({ '--Grid-rowSpacing': '16px', + '> *': { + '--Grid-parent-rowSpacing': '16px', + }, '@media (min-width:900px)': { '--Grid-rowSpacing': '24px', + '> *': { + '--Grid-parent-rowSpacing': '24px', + }, }, '@media (min-width:1536px)': { '--Grid-rowSpacing': '0px', + '> *': { + '--Grid-parent-rowSpacing': '0px', + }, }, }); @@ -196,22 +229,23 @@ describe('grid generator', () => { }), ).to.deep.equal({ '--Grid-rowSpacing': '0px', + '> *': { + '--Grid-parent-rowSpacing': '0px', + }, '@media (min-width:900px)': { '--Grid-rowSpacing': '16px', + '> *': { + '--Grid-parent-rowSpacing': '16px', + }, }, '@media (min-width:1536px)': { '--Grid-rowSpacing': '0px', + '> *': { + '--Grid-parent-rowSpacing': '0px', + }, }, }); }); - - it('nested item level 1 should have default spacing set to parent', () => { - const result = generateGridRowSpacingStyles({ - theme: { breakpoints }, - ownerState: { container: true, unstable_level: 1 }, - }); - expect(result['--Grid-rowSpacingLevel1']).to.equal('var(--Grid-rowSpacing)'); - }); }); describe('generateGridColumnSpacingStyles', () => { @@ -223,6 +257,9 @@ describe('grid generator', () => { }), ).to.deep.equal({ '--Grid-columnSpacing': '16px', + '> *': { + '--Grid-parent-columnSpacing': '16px', + }, }); }); @@ -234,6 +271,9 @@ describe('grid generator', () => { }), ).to.deep.equal({ '--Grid-columnSpacing': '1rem', + '> *': { + '--Grid-parent-columnSpacing': '1rem', + }, }); }); @@ -245,11 +285,20 @@ describe('grid generator', () => { }), ).to.deep.equal({ '--Grid-columnSpacing': '16px', + '> *': { + '--Grid-parent-columnSpacing': '16px', + }, '@media (min-width:900px)': { '--Grid-columnSpacing': '24px', + '> *': { + '--Grid-parent-columnSpacing': '24px', + }, }, '@media (min-width:1536px)': { '--Grid-columnSpacing': '0px', + '> *': { + '--Grid-parent-columnSpacing': '0px', + }, }, }); @@ -260,22 +309,23 @@ describe('grid generator', () => { }), ).to.deep.equal({ '--Grid-columnSpacing': '0px', + '> *': { + '--Grid-parent-columnSpacing': '0px', + }, '@media (min-width:900px)': { '--Grid-columnSpacing': '16px', + '> *': { + '--Grid-parent-columnSpacing': '16px', + }, }, '@media (min-width:1536px)': { '--Grid-columnSpacing': '0px', + '> *': { + '--Grid-parent-columnSpacing': '0px', + }, }, }); }); - - it('nested item level 1 should have default spacing set to parent', () => { - const result = generateGridColumnSpacingStyles({ - theme: { breakpoints }, - ownerState: { container: true, unstable_level: 1 }, - }); - expect(result['--Grid-columnSpacingLevel1']).to.equal('var(--Grid-columnSpacing)'); - }); }); describe('generateGridOffsetStyles', () => { @@ -288,7 +338,7 @@ describe('grid generator', () => { ).to.deep.equal({ marginLeft: '0px', '@media (min-width:900px)': { - marginLeft: `calc(100% * 5 / var(--Grid-columns) + var(--Grid-columnSpacing) * 5 / var(--Grid-columns))`, + marginLeft: `calc(100% * 5 / var(--Grid-parent-columns) + var(--Grid-parent-columnSpacing) * 5 / var(--Grid-parent-columns))`, }, '@media (min-width:1200px)': { marginLeft: `auto`, diff --git a/packages/mui-system/src/Grid/gridGenerator.ts b/packages/mui-system/src/Grid/gridGenerator.ts index 935c2a78857c89..7a997c71f507a6 100644 --- a/packages/mui-system/src/Grid/gridGenerator.ts +++ b/packages/mui-system/src/Grid/gridGenerator.ts @@ -9,41 +9,18 @@ interface Props { ownerState: GridOwnerState; } -function appendLevel(level: number | undefined) { - if (!level) { - return ''; - } - return `Level${level}`; +function getSelfSpacingVar(axis: 'row' | 'column') { + return `--Grid-${axis}Spacing`; } -function isNestedContainer(ownerState: Props['ownerState']) { - return ownerState.unstable_level > 0 && ownerState.container; +function getParentSpacingVar(axis: 'row' | 'column') { + return `--Grid-parent-${axis}Spacing`; } -function createGetSelfSpacing(ownerState: Props['ownerState']) { - return function getSelfSpacing(axis: 'row' | 'column') { - return `var(--Grid-${axis}Spacing${appendLevel(ownerState.unstable_level)})`; - }; -} - -function createGetParentSpacing(ownerState: Props['ownerState']) { - return function getParentSpacing(axis: 'row' | 'column') { - if (ownerState.unstable_level === 0) { - return `var(--Grid-${axis}Spacing)`; - } - return `var(--Grid-${axis}Spacing${appendLevel(ownerState.unstable_level - 1)})`; - }; -} - -function getParentColumns(ownerState: Props['ownerState']) { - if (ownerState.unstable_level === 0) { - return `var(--Grid-columns)`; - } - return `var(--Grid-columns${appendLevel(ownerState.unstable_level - 1)})`; -} +const selfColumnsVar = '--Grid-columns'; +const parentColumnsVar = '--Grid-parent-columns'; export const generateGridSizeStyles = ({ theme, ownerState }: Props) => { - const getParentSpacing = createGetParentSpacing(ownerState); const styles = {}; traverseBreakpoints<'auto' | 'grow' | number | false>( theme.breakpoints, @@ -70,7 +47,7 @@ export const generateGridSizeStyles = ({ theme, ownerState }: Props) => { style = { flexGrow: 0, flexBasis: 'auto', - width: `calc(100% * ${value} / ${getParentColumns(ownerState)} - (${getParentColumns(ownerState)} - ${value}) * (${getParentSpacing('column')} / ${getParentColumns(ownerState)}))`, + width: `calc(100% * ${value} / var(${parentColumnsVar}) - (var(${parentColumnsVar}) - ${value}) * (var(${getParentSpacingVar('column')}) / var(${parentColumnsVar})))`, }; } appendStyle(styles, style); @@ -80,7 +57,6 @@ export const generateGridSizeStyles = ({ theme, ownerState }: Props) => { }; export const generateGridOffsetStyles = ({ theme, ownerState }: Props) => { - const getParentSpacing = createGetParentSpacing(ownerState); const styles = {}; traverseBreakpoints( theme.breakpoints, @@ -97,7 +73,7 @@ export const generateGridOffsetStyles = ({ theme, ownerState }: Props) => { marginLeft: value === 0 ? '0px' - : `calc(100% * ${value} / ${getParentColumns(ownerState)} + ${getParentSpacing('column')} * ${value} / ${getParentColumns(ownerState)})`, + : `calc(100% * ${value} / var(${parentColumnsVar}) + var(${getParentSpacingVar('column')}) * ${value} / var(${parentColumnsVar}))`, }; } appendStyle(styles, style); @@ -110,34 +86,36 @@ export const generateGridColumnsStyles = ({ theme, ownerState }: Props) => { if (!ownerState.container) { return {}; } - const styles = isNestedContainer(ownerState) - ? { [`--Grid-columns${appendLevel(ownerState.unstable_level)}`]: getParentColumns(ownerState) } - : { '--Grid-columns': 12 }; + const styles = { + [selfColumnsVar]: 12, + }; traverseBreakpoints(theme.breakpoints, ownerState.columns, (appendStyle, value) => { - appendStyle(styles, { [`--Grid-columns${appendLevel(ownerState.unstable_level)}`]: value }); + const columns = value ?? 12; + appendStyle(styles, { + [selfColumnsVar]: columns, + '> *': { + [parentColumnsVar]: columns, + }, + }); }); - return styles as Record; + return styles; }; export const generateGridRowSpacingStyles = ({ theme, ownerState }: Props) => { if (!ownerState.container) { return {}; } - const getParentSpacing = createGetParentSpacing(ownerState); - const styles = isNestedContainer(ownerState) - ? { - // Set the default spacing as its parent spacing. - // It will be overridden if spacing props are provided - [`--Grid-rowSpacing${appendLevel(ownerState.unstable_level)}`]: getParentSpacing('row'), - } - : {}; + const styles = {}; traverseBreakpoints( theme.breakpoints, ownerState.rowSpacing, (appendStyle, value) => { + const spacing = typeof value === 'string' ? value : theme.spacing?.(value); appendStyle(styles, { - [`--Grid-rowSpacing${appendLevel(ownerState.unstable_level)}`]: - typeof value === 'string' ? value : theme.spacing?.(value), + [getSelfSpacingVar('row')]: spacing, + '> *': { + [getParentSpacingVar('row')]: spacing, + }, }); }, ); @@ -148,22 +126,17 @@ export const generateGridColumnSpacingStyles = ({ theme, ownerState }: Props) => if (!ownerState.container) { return {}; } - const getParentSpacing = createGetParentSpacing(ownerState); - const styles = isNestedContainer(ownerState) - ? { - // Set the default spacing as its parent spacing. - // It will be overridden if spacing props are provided - [`--Grid-columnSpacing${appendLevel(ownerState.unstable_level)}`]: - getParentSpacing('column'), - } - : {}; + const styles = {}; traverseBreakpoints( theme.breakpoints, ownerState.columnSpacing, (appendStyle, value) => { + const spacing = typeof value === 'string' ? value : theme.spacing?.(value); appendStyle(styles, { - [`--Grid-columnSpacing${appendLevel(ownerState.unstable_level)}`]: - typeof value === 'string' ? value : theme.spacing?.(value), + [getSelfSpacingVar('column')]: spacing, + '> *': { + [getParentSpacingVar('column')]: spacing, + }, }); }, ); @@ -186,7 +159,6 @@ export const generateGridDirectionStyles = ({ theme, ownerState }: Props) => { }; export const generateGridStyles = ({ ownerState }: Props): {} => { - const getSelfSpacing = createGetSelfSpacing(ownerState); return { minWidth: 0, boxSizing: 'border-box', @@ -197,7 +169,7 @@ export const generateGridStyles = ({ ownerState }: Props): {} => { ownerState.wrap !== 'wrap' && { flexWrap: ownerState.wrap, }), - gap: `${getSelfSpacing('row')} ${getSelfSpacing('column')}`, + gap: `var(${getSelfSpacingVar('row')}) var(${getSelfSpacingVar('column')})`, }), }; }; diff --git a/test/regressions/fixtures/Grid/Issue43707.js b/test/regressions/fixtures/Grid/Issue43707.js new file mode 100644 index 00000000000000..66daf1955f6d07 --- /dev/null +++ b/test/regressions/fixtures/Grid/Issue43707.js @@ -0,0 +1,79 @@ +/* eslint-disable */ +import * as React from 'react'; +import { styled } from '@mui/material/styles'; +import Box from '@mui/material/Box'; +import Paper from '@mui/material/Paper'; +import Grid from '@mui/material/Grid2'; + +const Item = styled(Paper)(({ theme }) => ({ + backgroundColor: '#fff', + ...theme.typography.body2, + padding: theme.spacing(1), + textAlign: 'center', + color: theme.palette.text.secondary, + ...theme.applyStyles('dark', { + backgroundColor: '#1A2027', + }), +})); + +export default function VariableWidthGrid() { + return ( + + + + size=auto + + + size=6 + + + size=grow + + + + + Nested 10 under a 12 + + + Nested 2 under a 12 + + + + + + Nested 10 under a 12 override spacing + + + Nested 2 under a 12 override spacing + + + + + + {/* Container not nested in container. Does not inherit spacing */} + + Incorrectly nested 10 under a 12 + + + Incorrectly nested 2 under a 12 + + + + + + <> + + {/* Container not nested in container. Does not inherit spacing */} + + Nested 10 under a 12 with fragment + + + Nested 2 under a 12 with fragment + + + + + + + ); +}