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

[grid] Fix column spacing for nested containers #43733

Merged
merged 21 commits into from
Oct 3, 2024
Merged

[grid] Fix column spacing for nested containers #43733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
bac713f
[grid] Fix column spacing for nested containers
Janpot Sep 12, 2024
076f06a
Update gridGenerator.ts
Janpot Sep 12, 2024
b079d6f
Update gridGenerator.ts
Janpot Sep 12, 2024
0ecf6fc
Update gridGenerator.ts
Janpot Sep 12, 2024
1dbad7d
Update gridGenerator.ts
Janpot Sep 12, 2024
983ac7d
Update gridGenerator.ts
Janpot Sep 12, 2024
bfceae4
Update gridGenerator.ts
Janpot Sep 12, 2024
92c95f1
Create Issue43707.js
Janpot Sep 12, 2024
96f39f0
Update tests
Janpot Sep 12, 2024
e833ef2
Update Issue43707.js
Janpot Sep 12, 2024
96ecee5
Update Issue43707.js
Janpot Sep 12, 2024
04ab8ee
Update Issue43707.js
Janpot Sep 12, 2024
a2d59b9
Update Issue43707.js
Janpot Sep 12, 2024
264d5d5
Update Issue43707.js
Janpot Sep 12, 2024
d4a9ebe
Merge remote-tracking branch 'upstream/master' into nested-grid
Janpot Sep 13, 2024
bcaf42b
Mark pure
Janpot Sep 13, 2024
be183c4
Update gridGenerator.ts
Janpot Sep 13, 2024
421c8da
Merge remote-tracking branch 'upstream/master' into nested-grid
Janpot Sep 17, 2024
483f915
Update comment
Janpot Sep 17, 2024
19f4862
Merge remote-tracking branch 'upstream/master' into nested-grid
Janpot Oct 3, 2024
28508f9
Update PigmentGrid.tsx
Janpot Oct 3, 2024
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
54 changes: 32 additions & 22 deletions packages/mui-material/src/Grid2/Grid2.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,24 +52,29 @@ export interface GridBaseProps {
offset?: ResponsiveStyleValue<GridOffset>;
/**
* @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
* <Grid> // level 0
* <Grid> // level 1
* <Grid> // level 2
* <Grid> // level 1
* <Grid container> // level 0
* <Grid container> // level 1
* <Grid container> // 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
* <Grid> // level 0
* <Grid container> // level 0
* <div>
* <Grid> // level 0
* <Grid> // level 1
* <Grid container> // level 0
* ```
*
* ```js
* <Grid container> // level 0
* <Grid>
* <Grid container> // level 0
* ```
*/
unstable_level?: number;
Expand Down Expand Up @@ -222,24 +227,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
* <Grid> // level 0
* <Grid> // level 1
* <Grid> // level 2
* <Grid> // level 1
* <Grid container> // level 0
* <Grid container> // level 1
* <Grid container> // 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
* <Grid> // level 0
* <Grid container> // level 0
* <div>
* <Grid> // level 0
* <Grid> // level 1
* <Grid container> // level 0
* ```
*
* ```js
* <Grid container> // level 0
* <Grid>
* <Grid container> // level 0
* ```
*/
unstable_level: PropTypes.number,
Expand Down
46 changes: 0 additions & 46 deletions packages/mui-material/src/PigmentGrid/PigmentGrid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,29 +55,6 @@ export interface GridBaseProps {
* Defines the offset of the grid.
*/
offset?: ResponsiveStyleValue<number> | 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
* <Grid> // level 0
* <Grid> // level 1
* <Grid> // level 2
* <Grid> // level 1
* ```
*
* Only consecutive grid is considered nesting.
* A grid container will start at `0` if there are non-Grid element above it.
*
* ```js
* <Grid> // level 0
* <div>
* <Grid> // level 0
* <Grid> // level 1
* ```
*/
unstable_level?: number;
/**
* Defines the vertical space between the type `item` components.
* It overrides the value of the `spacing` prop.
Expand Down Expand Up @@ -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
* <Grid> // level 0
* <Grid> // level 1
* <Grid> // level 2
* <Grid> // level 1
* ```
*
* Only consecutive grid is considered nesting.
* A grid container will start at `0` if there are non-Grid element above it.
*
* ```js
* <Grid> // level 0
* <div>
* <Grid> // level 0
* <Grid> // level 1
* ```
*/
unstable_level: PropTypes.number,
/**
* Defines the `flex-wrap` style property.
* It's applied for all screen sizes.
Expand Down
27 changes: 16 additions & 11 deletions packages/mui-system/src/Grid/Grid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
* <Grid> // level 0
* <Grid> // level 1
* <Grid> // level 2
* <Grid> // level 1
* <Grid container> // level 0
* <Grid container> // level 1
* <Grid container> // 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
* <Grid> // level 0
* <Grid container> // level 0
* <div>
* <Grid> // level 0
* <Grid> // level 1
* <Grid container> // level 0
* ```
*
* ```js
* <Grid container> // level 0
* <Grid>
* <Grid container> // level 0
* ```
*/
unstable_level: PropTypes.number,
Expand Down
27 changes: 16 additions & 11 deletions packages/mui-system/src/Grid/GridProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,24 +49,29 @@ export interface GridBaseProps {
offset?: ResponsiveStyleValue<GridOffset>;
/**
* @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
* <Grid> // level 0
* <Grid> // level 1
* <Grid> // level 2
* <Grid> // level 1
* <Grid container> // level 0
* <Grid container> // level 1
* <Grid container> // 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
* <Grid> // level 0
* <Grid container> // level 0
* <div>
* <Grid> // level 0
* <Grid> // level 1
* <Grid container> // level 0
* ```
*
* ```js
* <Grid container> // level 0
* <Grid>
* <Grid container> // level 0
* ```
*/
unstable_level?: number;
Expand Down
7 changes: 6 additions & 1 deletion packages/mui-system/src/Grid/createGrid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Copy link
Member

Choose a reason for hiding this comment

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

I don't think checking for container is correct: https://github.com/mui/material-ui/blob/be183c4d560a8343f82c284dd407f618b54f06a2/packages/mui-system/src/Grid/Grid.tsx#L111C6-L128C29. The level should be increased regardless.

Copy link
Member Author

@Janpot Janpot Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The actual level number has no meaning anymore after this change. The only way it now is being used is to check whether it's either 0 or not. It's now only used to find out whether the current grid is nested (as per what the docs define as nested, container inside of container) or top-level. The new logic doesn't work anymore if you remove this condition. I believe we need to update that comment you refer to instead 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Oh, ok, that makes sense. Yes, if the logic is changed, let's update the comment.

child.props.container
) {
return React.cloneElement(child, {
unstable_level: (child.props as GridProps)?.unstable_level ?? level + 1,
} as GridProps);
Expand Down
Loading