[Grid2] Replace context with `cloneElement` #36399

siriwatknp · 2023-03-02T11:24:30Z

BREAKING CHANGE

Grid2 now uses React.cloneElement (was React context) to pass the spacing and columns to the next container. The change is close to how CSS flexbox behaves.

closes #36372, closes #36777 visual regression added.

Before

https://stackblitz.com/edit/react-ts-v9cb4z?file=App.tsx

After

https://codesandbox.io/s/material-cra-ts-forked-h3qicw?file=/src/App.tsx

I have followed (at least) the PR section of the contributing guide.

mui-bot · 2023-03-02T11:27:13Z

Netlify deploy preview

docs/data/material/components/grid2/grid2.md

@material-ui/core: parsed: +0.06% , gzip: +0.06%
@material-ui/system: parsed: +0.52% , gzip: +0.43%
@mui/joy: parsed: +0.08% , gzip: +0.08%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 44a703f

omermizr · 2023-03-03T10:49:51Z

Hey @siriwatknp, thanks for looking into this!
However, I tried this out with a few more test cases and found some issues. Here's the demo: https://codesandbox.io/s/material-cra-ts-forked-6hnmq4?file=/src/App.tsx
And more in depth:

A container that's a direct child of another container, but no child grids items has negative margins applied, but no padding. I'm not sure if this is just how it's supposed to work, but with the standard Grid (at least in v4) it was ok. Here the content looks off. (see Item 3 in the demo)
A container that's a direct child of another container and has a child grid item has extra padding applied to the top. (see Item 4 in the demo)
I don't think the last grid item should have a bottom padding (the first item doesn't have top padding), but I'm not sure about that. If you look at my example it seems pretty arbitrary. (see Item 5 in the demo)

Thanks again!

ghost · 2023-03-10T15:02:41Z

Is having no padding and spacing the intended way ? When do you think it will be fixed ?

siriwatknp · 2023-03-20T05:10:32Z

@omermizr It looks all correct to me.

(1) The usage is wrong. Grid containers and items should be used together, otherwise, there is no point in using the Grid component. We could make the docs more explicit on this point in https://mui.com/material-ui/react-grid2/#basic-grid.

(2, 3) Those padding is correct. The goal of the grid is to create consistent spacing between grid items. If you take a look at this https://codesandbox.io/s/material-cra-ts-forked-01y0gm?file=/src/App.tsx, you will see that the spacing between grid items is 24px which corresponds to the spacing={3}.

siriwatknp · 2023-03-20T06:36:08Z

Is having no padding and spacing the intended way ? When do you think it will be fixed ?

I guess you meant to use CSS flexbox gap instead of margin, and padding? It is not possible with the current Grid APIs (I have tried but failed).

It is definitely better to have Flexbox Grid using gap but it must be a different component and different API (not with container and item).

mnajdova · 2023-03-20T07:51:58Z

packages/mui-system/src/Unstable_Grid/GridProps.ts

+   *       <Grid> // level 1
+   * ```
+   */
+  level?: number;


What's the motivation of making this a public API? If we don't want it to be public, maybe we should prefix it with internal_.

Ping on this question, it's still not clear to me.

Okay, I will make it unstable_ for consistency.

mnajdova · 2023-03-20T07:53:05Z

packages/mui-system/src/Unstable_Grid/createGrid.tsx

-      />
+      >
+        {React.Children.map(children, (child) => {
+          if (React.isValidElement(child) && isMuiElement(child, ['Grid'])) {


Will this work if the Grid is styled? Should we add a test for it?

👍 Good point. Visual regression added.

omermizr · 2023-03-20T08:17:24Z

@omermizr It looks all correct to me.

(1) The usage is wrong. Grid containers and items should be used together, otherwise, there is no point in using the Grid component. We could make the docs more explicit on this point in https://mui.com/material-ui/react-grid2/#basic-grid.

(2, 3) Those padding is correct. The goal of the grid is to create consistent spacing between grid items. If you take a look at this https://codesandbox.io/s/material-cra-ts-forked-01y0gm?file=/src/App.tsx, you will see that the spacing between grid items is 24px which corresponds to the spacing={3}.

Thanks @siriwatknp!
(1) - ok, I thought that might not be the correct usage but I wasn't sure because it worked fine with Grid in MUI 4. Yeah I think the docs could be more explicit about this.
(2) - I get it, so the padding comes from it being a grid item. That makes sense, but the current implementation doesn't feel ergonomically correct. If I create a simple Grid2, with one of the items being a container, the padding is all off and I'd have to change it explicitly in order for the spacing to be consistent. See this simplified example: https://codesandbox.io/s/material-cra-ts-forked-60ryz2 (Just 3 grid items, the middle one is a container, but they are all differently sized and padded).
(3) - This still seems inconsistent to me, especially because of your wording "between grid items". If I have 2 grid items, the first one doesn't seem to have any padding applied to the top (it actually does, but it gets offset by the negative margin), but the second one does have bottom padding. See here: https://codesandbox.io/s/material-cra-ts-forked-jvm2g0

…nested-grid-context

siriwatknp · 2023-03-20T10:06:10Z

(2) - I get it, so the padding comes from it being a grid item. That makes sense, but the current implementation doesn't feel ergonomically correct. If I create a simple Grid2, with one of the items being a container, the padding is all off and I'd have to change it explicitly in order for the spacing to be consistent. See this simplified example: https://codesandbox.io/s/material-cra-ts-forked-60ryz2 (Just 3 grid items, the middle one is a container, but they are all differently sized and padded).

Honestly, I don't understand your point on the padding is all off and I'd have to change it explicitly in order for the spacing to be consistent.

What do you have to change? because if I remove the background from your sandbox, the spacing looks correct https://codesandbox.io/s/material-cra-ts-forked-jcio79?file=/src/App.tsx.

If you really want to add backgrounds, you should add them to grid items (not containers).

(3) - This still seems inconsistent to me, especially because of your wording "between grid items". If I have 2 grid items, the first one doesn't seem to have any padding applied to the top (it actually does, but it gets offset by the negative margin), but the second one does have bottom padding. See here: https://codesandbox.io/s/material-cra-ts-forked-jvm2g0

My bad, it is not between grid items but between grid items' content. I hope this picture is clearer.

One thing I notice in all of your examples is direction="column". I don't see any reason to use Grid with direction="column", if you have a valid use case please share it.

siriwatknp · 2023-03-20T10:28:46Z

packages/mui-system/src/createStyled.js

+      if (tag.muiName) {
+        Component.muiName = tag.muiName;
+      }


@mnajdova This is required to make styled(Grid2) works. Some Joy UI components would benefit from this fix as well.

omermizr · 2023-03-20T10:30:09Z

Honestly, I don't understand your point on the padding is all off and I'd have to change it explicitly in order for the spacing to be consistent.

What do you have to change? because if I remove the background from your sandbox, the spacing looks correct https://codesandbox.io/s/material-cra-ts-forked-jcio79?file=/src/App.tsx.

If you really want to add backgrounds, you should add them to grid items (not containers).

Ok yes, if we remove the background color the Grid items look better, but does that mean we shouldn't expect styling to behave "normally"? If I need to set a background color for a container, how would I go about doing that? And background color is one thing, but I suspect there would be other things (like borders?) that might feel strange.

My bad, it is not between grid items but between grid items' content. I hope this picture is clearer.

But my issue is that the last grid item content has extra spacing after it, but the first one doesn't have extra spacing before it... Even without background color, it just means the next element in the dom would be further away for no apparent reason. Maybe this clarifies it:

This might be the same issue as before actually, because the negative margin means the next dom element would be affected by the negative margin 🤔

siriwatknp · 2023-03-21T11:51:34Z

Ok yes, if we remove the background color the Grid items look better, but does that mean we shouldn't expect styling to behave "normally"? If I need to set a background color for a container, how would I go about doing that? And background color is one thing, but I suspect there would be other things (like borders?) that might feel strange.

You might be able to style the root grid container but not a nested container. I think styling grid items should be the first option:

<Grid container sx={{ '& > .MuiGrid2-root:not(.MuiGrid2-container)': {
  backgroundColor: 'red',
}}}>
  <Grid>…</Grid>
  <Grid>…</Grid>
</Grid>

But my issue is that the last grid item content has extra spacing after it, but the first one doesn't have extra spacing before it... Even without background color, it just means the next element in the dom would be further away for no apparent reason. Maybe this clarifies it:

Now I got it. I don't think that's an issue. It is the effect of a negative margin that behaves the same since Material UI v4. You could say that it is a limitation, so you have to manually add spacing to the next element of the grid.

<Grid container spacing={3}> … </Grid>
<Box sx={{ mt: 1.5 }}>test</Box>

…nested-grid-context

siriwatknp · 2023-04-05T04:03:19Z

This will fix #36777

Proof: https://codesandbox.io/s/material-cra-forked-hc042i?file=/src/App.js

siriwatknp · 2023-04-10T05:53:57Z

@mnajdova I think this PR is ready to go.

mnajdova

Two comments, the logic looks good 👍

mnajdova · 2023-04-14T09:19:04Z

docs/data/material/components/grid2/grid2.md


-A nested grid container will inherits the columns from its parent unless the `columns` prop is specified to the instance.
+Note that a nested grid container should be a direct child of another grid container. If there are non-grid elements in between, the grid container will start as the new root container.


This makes much more sense 👍 Note, we need to add a BREAKING CHANGE section in the PR description (and in the chalengelog entry).

mnajdova · 2023-04-14T09:19:29Z

packages/mui-system/src/Unstable_Grid/GridProps.ts

+   *       <Grid> // level 1
+   * ```
+   */
+  level?: number;


Ping on this question, it's still not clear to me.

mnajdova · 2023-04-14T09:27:28Z

test/regressions/fixtures/Grid/StressNestedGrid2.js

+            </Grid>
+          </Grid>
+        </Box>
+        <Grid container xs={6} spacing={3}>


Suggested change

<Grid container xs={6} spacing={3}>

<Grid container xs={6}>

Should we remove the spacing from some of these children and test that it will be inherited from the parent?

…nested-grid-context

siriwatknp added 2 commits March 2, 2023 17:36

replace context with cloneElement

81cb9b4

add regression

28288a7

siriwatknp added component: Grid The React component. package: system Specific to @mui/system labels Mar 2, 2023

add docs

f56f68e

siriwatknp marked this pull request as ready for review March 3, 2023 04:23

siriwatknp mentioned this pull request Mar 3, 2023

Grid2 nested container inherits padding and spacing #36372

Closed

2 tasks

update regression

ba21918

siriwatknp added the bug 🐛 Something doesn't work label Mar 3, 2023

siriwatknp requested review from mnajdova and a team March 3, 2023 04:38

mnajdova reviewed Mar 20, 2023

View reviewed changes

Merge branch 'master' of https://github.com/mui/material-ui into fix/…

471bb7a

…nested-grid-context

siriwatknp added 2 commits March 20, 2023 17:11

styled component should append muiName

f6e0548

fix regression

6ecf2f7

siriwatknp commented Mar 20, 2023

View reviewed changes

siriwatknp requested a review from mnajdova March 21, 2023 11:54

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 22, 2023

Merge branch 'master' of https://github.com/mui/material-ui into fix/…

9621a61

…nested-grid-context

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 3, 2023

siriwatknp mentioned this pull request Apr 5, 2023

[Grid][Dialog] Using grid and dialog together doesn't work #36777

Closed

2 tasks

mnajdova approved these changes Apr 14, 2023

View reviewed changes

siriwatknp added 3 commits April 17, 2023 10:38

Merge branch 'master' of https://github.com/mui/material-ui into fix/…

367c028

…nested-grid-context

mark level as unstable

da2fc6b

update regression test

d3f7f97

siriwatknp added the breaking change label Apr 17, 2023

trigger build

44a703f

siriwatknp merged commit 7310533 into mui:master Apr 17, 2023

siriwatknp mentioned this pull request Jul 3, 2023

Confusion regarding Grid2 inheritance #37748

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Grid2] Replace context with `cloneElement` #36399

[Grid2] Replace context with `cloneElement` #36399

siriwatknp commented Mar 2, 2023 •

edited

Loading

mui-bot commented Mar 2, 2023 •

edited

Loading

omermizr commented Mar 3, 2023 •

edited

Loading

ghost commented Mar 10, 2023

siriwatknp commented Mar 20, 2023

siriwatknp commented Mar 20, 2023

mnajdova Mar 20, 2023

mnajdova Apr 14, 2023

siriwatknp Apr 17, 2023

mnajdova Mar 20, 2023

siriwatknp Mar 20, 2023

omermizr commented Mar 20, 2023

siriwatknp commented Mar 20, 2023 •

edited

Loading

siriwatknp Mar 20, 2023 •

edited

Loading

omermizr commented Mar 20, 2023

siriwatknp commented Mar 21, 2023 •

edited

Loading

siriwatknp commented Apr 5, 2023

siriwatknp commented Apr 10, 2023

mnajdova left a comment

mnajdova Apr 14, 2023

mnajdova Apr 14, 2023

mnajdova Apr 14, 2023


		A nested grid container will inherits the columns from its parent unless the `columns` prop is specified to the instance.
		Note that a nested grid container should be a direct child of another grid container. If there are non-grid elements in between, the grid container will start as the new root container.

[Grid2] Replace context with cloneElement #36399

[Grid2] Replace context with cloneElement #36399

Conversation

siriwatknp commented Mar 2, 2023 • edited Loading

mui-bot commented Mar 2, 2023 • edited Loading

Netlify deploy preview

Bundle size report

omermizr commented Mar 3, 2023 • edited Loading

ghost commented Mar 10, 2023

siriwatknp commented Mar 20, 2023

siriwatknp commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omermizr commented Mar 20, 2023

siriwatknp commented Mar 20, 2023 • edited Loading

siriwatknp Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

omermizr commented Mar 20, 2023

siriwatknp commented Mar 21, 2023 • edited Loading

siriwatknp commented Apr 5, 2023

siriwatknp commented Apr 10, 2023

mnajdova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Grid2] Replace context with `cloneElement` #36399

[Grid2] Replace context with `cloneElement` #36399

siriwatknp commented Mar 2, 2023 •

edited

Loading

mui-bot commented Mar 2, 2023 •

edited

Loading

omermizr commented Mar 3, 2023 •

edited

Loading

siriwatknp commented Mar 20, 2023 •

edited

Loading

siriwatknp Mar 20, 2023 •

edited

Loading

siriwatknp commented Mar 21, 2023 •

edited

Loading