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.

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

[Grid2] Add dynamic classes to Grid2Classes interface #44572

Closed
wants to merge 2 commits into from

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Nov 27, 2024

This PR adds dynamic classes to Grid2Classes interface which were reverted in this PR #44420

WIP, Don't review yet

@sai6855 sai6855 added component: Grid The React component. typescript labels Nov 27, 2024
@mui-bot
Copy link

mui-bot commented Nov 27, 2024

Netlify deploy preview

https://deploy-preview-44572--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 2ce649a

@sai6855 sai6855 requested a review from DiegoAndai November 27, 2024 12:02
Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

Should these classes be documented? I think the classes we document should be the ones that are considered public API and are meant to be targeted by users. Also, users can modify the breakpoints which means these classes can potentially become non-relevant.

@sai6855
Copy link
Contributor Author

sai6855 commented Nov 27, 2024

Should these classes be documented?

Grid doesn't document dynamic classes, so i wan't sure if those need to be documentd in Grid2
Grid classes: https://mui.com/material-ui/api/grid/#classes

Also, users can modify the breakpoints which means these classes can potentially become non-relevant.

Correct. The classes added in this PR simply reflect the ones that are actually added to the DOM. However, I think the bigger issue is that if users modify their breakpoints, neither Grid nor Grid2 handles this scenario properly, as the breakpoints are hardcoded in both components.

...SPACINGS.map((spacing) => `spacing-xs-${spacing}` as const),
// direction values
...DIRECTIONS.map((direction) => `direction-xs-${direction}` as const),
// wrap values
...WRAPS.map((wrap) => `wrap-xs-${wrap}` as const),
// grid sizes for all breakpoints
...GRID_SIZES.map((size) => `grid-xs-${size}` as const),
...GRID_SIZES.map((size) => `grid-sm-${size}` as const),
...GRID_SIZES.map((size) => `grid-md-${size}` as const),
...GRID_SIZES.map((size) => `grid-lg-${size}` as const),

@sai6855 sai6855 marked this pull request as draft November 29, 2024 09:44
@aarongarciah
Copy link
Member

@sai6855 do you think this PR still makes sense? If not, let's close it.

@sai6855
Copy link
Contributor Author

sai6855 commented Dec 26, 2024

@sai6855 do you think this PR still makes sense? If not, let's close it.

Given breakpoints can be dynamic, don't think this PR is relevant

@sai6855 sai6855 closed this Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Grid The React component. typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants