[Grid] Support customized breakpoints #31998
Conversation
boutahlilsoufiane
changed the title
…grid-support-customized-breakpoints
github-actions
bot
added
the
PR: out-of-date
…grid-support-customized-breakpoints
github-actions
bot
removed
the
PR: out-of-date
There was a problem hiding this comment.
The changes look good, I've left one comment for a change that we need to revert. Could you please add tests for this behavior? You can add a test in the moduleAugmentation suite. You can call it something like gridCustomBreakpoints.spec.tsx
|
Hi @mnajdova, thanks a lot for your time, I will check the changes! |
|
Hi @mnajdova thanks a lot for taking the time to review the PR, could you please review it? |
|
Hi @mnajdova, thanks a lot for taking the time to clarify things, I appreciate your precious time. The comments are addressed! |
There was a problem hiding this comment.
One last comment from my side. Taggin @siriwatknp for a final review
|
|
||
| return [ | ||
| styles.root, | ||
| container && styles.container, | ||
| item && styles.item, | ||
| zeroMinWidth && styles.zeroMinWidth, | ||
| ...resolveSpacingClasses(spacing, container, styles), | ||
| ...spacingClasses, |
There was a problem hiding this comment.
This doesn't make much sense, why spreading spacing classes here? Based on the logic inside resolveSpacingClasses if styles do not exists for the specific key, a class name is being added.
Not saying is a fault of this PR, but it is wrong. Maybe we should have two different functions, one for classes, and one for adding the styles overrides.
| const slots = { | ||
| root: [ | ||
| 'root', | ||
| container && 'container', | ||
| item && 'item', | ||
| zeroMinWidth && 'zeroMinWidth', | ||
| ...resolveSpacingClasses(spacing, container), | ||
| ...spacingClasses, |
There was a problem hiding this comment.
We would use the classes util here only
|
@boutahlilsoufiane what was this: 39dc30f ? 😅 |
|
Hi @mnajdova, thanks a lot for your time. Thanks for removing the code! |
oliviertassinari
changed the title
|
This PR introduces a regression. It can be seen on https://mui.com/material-ui/react-grid/. It can be reproduced with: <Grid key={12} />
which is because of that is because of the changes inside Grid.js that get intercepted in a strange way by |
Fix #26369.
This fixes the problem of passing custom breakpoints as props to the component Grid, the TypeScript throws an error when we pass these dynamic props and that happens because we are not supporting custom breakpoints in TypeScript.