[grid] Fix column spacing for nested containers#43733
Conversation
Janpot
added
type: bug
Netlify deploy previewhttps://deploy-preview-43733--material-ui.netlify.app/ @mui/joy: parsed: -0.11% 😍, gzip: -0.10% 😍 Bundle size reportDetails of bundle changes (Toolpad) |
DiegoAndai
left a comment
DiegoAndai
left a comment
There was a problem hiding this comment.
Thanks fro working on this @Janpot!
| if ( | ||
| React.isValidElement(child) && | ||
| isMuiElement(child, ['Grid']) && | ||
| container && |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
Oh, ok, that makes sense. Yes, if the logic is changed, let's update the comment.
DiegoAndai
left a comment
DiegoAndai
left a comment
There was a problem hiding this comment.
One final minor comment, but otherwise looks good 🙌🏼
I think we can use the same approach for the Pigment Grid to simplify its implementation.
| * <Grid container> // level 0 | ||
| * ``` | ||
| */ | ||
| unstable_level?: number; |
There was a problem hiding this comment.
The PigmentGrid doesn't have the unstable_level prop (reference), it shouldn't have been added. May I ask you to remove it? (or if you prefer, I can do it in a separate PR, let me know)
3 participants
...LevelXvariable doesn't behave correctly. It creates correct gaps on containers, but children don't use the correct variable to calculate their width. The JS logic seems sound though, I just translated it to CSS with a-parent-variable on the container children. It behaves a lot better.Closes #43707