[grid] Remove 'column' and 'column-reverse' options from GridDirection type#47564
Conversation
Netlify deploy previewhttps://deploy-preview-47564--material-ui.netlify.app/ Bundle size report
|
There was a problem hiding this comment.
Pull request overview
This PR removes 'column' and 'column-reverse' options from the GridDirection type definition, limiting it to only 'row' and 'row-reverse' values.
- Changes the exported GridDirection type to exclude column-based flex-direction values
- Represents a breaking change to the Grid component's TypeScript API
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type ResponsiveStyleValue<T> = T | Array<T | null> | { [key in Breakpoint]?: T | null }; | ||
|
|
||
| export type GridDirection = 'row' | 'row-reverse' | 'column' | 'column-reverse'; | ||
| export type GridDirection = 'row' | 'row-reverse'; |
There was a problem hiding this comment.
This is a breaking API change that removes valid CSS flex-direction values ('column' and 'column-reverse') from the GridDirection type. This will cause TypeScript compilation errors for any existing code using these direction values. If this is intentional, it should be:
- Documented as a breaking change
- Include a migration guide explaining why these values are being removed and what alternatives users should use
- Consider deprecating these values first in a minor release before removing them in a major version
If there's a technical reason why column directions shouldn't be supported in Grid, that rationale should be documented.
| export type GridDirection = 'row' | 'row-reverse'; | |
| export type GridDirection = 'row' | 'row-reverse' | 'column' | 'column-reverse'; |
| type ResponsiveStyleValue<T> = T | Array<T | null> | { [key in Breakpoint]?: T | null }; | ||
|
|
||
| export type GridDirection = 'row' | 'row-reverse' | 'column' | 'column-reverse'; | ||
| export type GridDirection = 'row' | 'row-reverse'; |
There was a problem hiding this comment.
The GridDirection type is being updated to remove 'column' and 'column-reverse' values, but this change is incomplete. Several other parts of the codebase still reference these values and need to be updated:
- PropTypes validation (lines 181-182 in this file) still accepts 'column' and 'column-reverse'
- The DIRECTIONS constant in gridClasses.ts (line 18) still includes all four values
- The underlying @mui/system/Grid/GridProps.ts still defines GridDirection with all four values
- PigmentGrid also has the full set of direction values
This incomplete change will cause inconsistencies between TypeScript type checking and runtime behavior, and may break applications that use column directions. Consider either completing all related changes together or reverting this change until a comprehensive update can be made.
| export type GridDirection = 'row' | 'row-reverse'; | |
| export type GridDirection = 'row' | 'row-reverse' | 'column' | 'column-reverse'; |
@siriwatknp applied suggestion as per your review, can you re-check? |
| <Grid container sx={{ justifyContent: 'flex-end' }} size={6}> | ||
| <Box sx={{ display: 'flex', flexDirection: 'column', alignItems: 'end' }}> | ||
| <Button onClick={handleClick('right-start')}>right-start</Button> | ||
| </Grid> | ||
| <Grid> | ||
|
|
||
| <Button onClick={handleClick('right')}>right</Button> | ||
| </Grid> | ||
| <Grid> | ||
|
|
||
| <Button onClick={handleClick('right-end')}>right-end</Button> | ||
| </Grid> | ||
| </Box> |
There was a problem hiding this comment.
This is wrong, let's change the Grid to Stack instead.
x the others.
| > | ||
| ≪ | ||
| </Button> | ||
| <Grid container sx={{ alignItems: 'center' }}> |
There was a problem hiding this comment.
I think Grid container is not needed
| <Grid container sx={{ alignItems: 'center' }}> |
|
@sai6855 Can you resolve this first? |
|
I just went through changes again, does this change can be treated as breaking change? If yes, is it okay to move ahead with this PR? |
Yes, it's fine as a breaking change |
|
@sai6855 can you update this PR? |
|
Since it's going to be a breaking change, it should also contain an entry in |
|
Hey @siriwatknp @silviuaavram, really sorry for the delay here. I’m currently not able to work on this PR or mui PRs for few days, so please feel free to close it or someone else can continue from here. |
No problem at all. @ZeeshanTamboli if you can take over this, let me know. |
|
@ZeeshanTamboli @siriwatknp merged master and changed the docs examples. feel free to take a look. |
siriwatknp
left a comment
There was a problem hiding this comment.
LGTM! One minor nit: PositionedPopper.js has sx={{ alignItems: 'flex-start' }} on the left-side Stack but PositionedPopper.tsx doesn't — they should be consistent.
|
Yeah just noticed it via the pipeline, changed it. Thanks! |
Try it here: https://stackblitz.com/edit/aytwvcfa?file=src%2FDemo.tsx
closes #47563
According to https://mui.com/material-ui/react-grid/#column-direction,
directionprop doesn't supportcolumnandcolumn-reversebut api-docs page listed them as valid . This PR fixes discrepancypreview: https://deploy-preview-47564--material-ui.netlify.app/material-ui/api/grid/