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

[material-ui][mui-system] Stabilize Grid v2 and deprecate Grid v1 #43054

Merged
merged 16 commits into from
Jul 30, 2024

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Jul 24, 2024

Closes: #42761

Stabilize Grid v2 in the material-ui and mui-system packages. This means:

  • Remove the Unstable_ prefix from the imports
  • Add Grid v2 API page, which didn't exist before
  • Add callouts in the v6 migration guide and Grid v2 migration guide about the removal of the prefix
  • Adjust the codemod to consider the updated imports.

Finally, deprecate the Grid v1 component.

Notes

  • Initially, I planned to change the Grid v1 name to DeprecatedGrid, but this defeats the purpose of deprecation as it introduces a breaking change (changing the import) instead of offering a deprecation period for users to adjust. We can remove the Grid v2 in v7 and finally rename the Grid v2 to Grid

  • I also planned to reroute the docs as such:

    • Grid v1: material-ui/deprecated-react-grid
    • Grid v2: material-ui/react-grid

    But this is a pain to do without changing the component's names. I don't think it's critical to make this change so I didn't do it.

@DiegoAndai DiegoAndai added breaking change component: Grid The React component. package: system Specific to @mui/system package: material-ui Specific to @mui/material labels Jul 24, 2024
@DiegoAndai DiegoAndai self-assigned this Jul 24, 2024
@DiegoAndai DiegoAndai changed the title [material-ui][mui-system] Stabilize Grid v2 and deprecate Grid v1. [material-ui][mui-system] Stabilize Grid v2 and deprecate Grid v1 Jul 24, 2024
@DiegoAndai
Copy link
Member Author

DiegoAndai commented Jul 24, 2024

Sorry for the large amount of files changed 😅 most of them are

-Unstable_Grid2
+Grid2

@mui-bot
Copy link

mui-bot commented Jul 24, 2024

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 25, 2024
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.

Just some very small comments. Looks great.

docs/data/system/components/grid/AutoGrid.tsx Outdated Show resolved Hide resolved
packages/mui-codemod/README.md Outdated Show resolved Hide resolved
packages/mui-material/src/Grid/Grid.d.ts Outdated Show resolved Hide resolved
packages/mui-material/src/Grid/Grid.js Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 25, 2024
@@ -1,7 +1,7 @@
import * as React from 'react';
import Avatar from '@mui/material/Avatar';
import Box from '@mui/material/Box';
import Grid from '@mui/material/Unstable_Grid2';
import Grid from '@mui/material/Grid2';
Copy link
Member

@siriwatknp siriwatknp Jul 26, 2024

Choose a reason for hiding this comment

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

Not related to this PR but I still see deprecated system props used in this demo. I found that there is no Unstable_Grid2 in the list of removeSystemProps codemod, it should be added along with Grid2 to the list.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll open a separate PR for this after we merge this one 👌🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

PR: #43302

- The spacing mechanism was reworked to use the `gap` CSS property.

This brings some breaking changes described in the following sections.

#### Stabilized API
Copy link
Member

@siriwatknp siriwatknp Jul 26, 2024

Choose a reason for hiding this comment

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

Suggested change
#### Stabilized API

I think this heading is not necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added sections because they're different breaking changes so providing structure makes it easier to follow, IMO. The other sections are for the size and offset props, and for the removal of disableEqualOverflow.


- The previous size and offset props were replaced with the `size` and `offset` props
- The previous size (`xs`, `sm`, `md`, ...) and offset (`xsOffset`, `smOffset`, `mdOffset`, ...) props, which were named after the theme's breakpoints, were replaced with the `size` and `offset` props.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add diff snippet to quickly show the migration?

@DiegoAndai
Copy link
Member Author

@siriwatknp about this comment:

Can you add diff snippet to quickly show the migration?

The diff snippets and details on how to migrate are on the "Size and offset props" section below, this is a summary of the changes. Do you think we shouldn't have the summary?

@DiegoAndai DiegoAndai requested a review from siriwatknp July 29, 2024 14:57
@siriwatknp
Copy link
Member

@siriwatknp about this comment:

Can you add diff snippet to quickly show the migration?

The diff snippets and details on how to migrate are on the "Size and offset props" section below, this is a summary of the changes. Do you think we shouldn't have the summary?

My bad, the diff is already in the section below that.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

🛳️ it.

@DiegoAndai DiegoAndai merged commit 08dce48 into mui:next Jul 30, 2024
22 checks passed
@DiegoAndai DiegoAndai deleted the stabilize-grid-v2 branch July 30, 2024 15:10
oliviertassinari added a commit that referenced this pull request Aug 2, 2024
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Great that we are consolidating on one approach. I imagine that Pigment CSS Grid is identical to Grid2, developers don't even need to know about it.

Thoughts:

  1. https://deploy-preview-43054--material-ui.netlify.app/material-ui/react-grid/ How about adding (deprecated) in the H1 to replicate the side nav? This would ensure Google and Algolia searches are clear.
  2. Do we have an issue to migrate the codebase away from the deprecated Grid? I think we will need one, e.g. for the community to help. Otherwise, it's an issue about removing the old API that we need, but same, to coordinate the work toward it.
  3. Continuing on 2. It feels like we should only add a runtime deprecation once we have migrated all our codebase. At that point, we would have a high confidence level that Grid v2 superset Grid v1.
  4. I'm confused about the names. Are @mui/material/Grid2 and @mui/system/Grid both Grid v2? If yes, then, please, please, let's make the name match. First because the different should only be about either the default theme is preset or not. And second because we are moving to a place where @mui/material/Grid2 and @mui/material/Grid don't exist as exports from this npm package.

Initially, I planned to change the Grid v1 name to DeprecatedGrid, but this defeats the purpose of deprecation as it introduces a breaking change (changing the import) instead of offering a deprecation period for users to adjust. We can remove the Grid v2 in v7 and finally rename the Grid v2 to Grid

  1. Interesting challenge, I didn't look close, I could have seen us go for a straight rename because 1. it's easy to handle for people 2. the API is almost the same, and we can have prop deprecation + props mapping when using the wrong imports to still get most of the behavior right + a clear migration message.

```

Alongside the stabilization, the API has been improved.
You can see the changes and further details of how to migrate in the [Material UI v6 migration guide](/material-ui/migrating-to-v6/).
Copy link
Member

Choose a reason for hiding this comment

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

This link doesn't work.

Suggested change
You can see the changes and further details of how to migrate in the [Material UI v6 migration guide](/material-ui/migrating-to-v6/).
You can see the changes and further details on how to migrate in the [Material UI v6 migration guide](/material-ui/migration/migrating-to-v6/).

Fixed in e36c897

Comment on lines +182 to +183
- import { Unstable_Grid2 as Grid2 } from '@mui/material';
+ import { Grid2 } from '@mui/material';
Copy link
Member

@oliviertassinari oliviertassinari Aug 2, 2024

Choose a reason for hiding this comment

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

Extra line break

Suggested change
- import { Unstable_Grid2 as Grid2 } from '@mui/material';
+ import { Grid2 } from '@mui/material';
-import { Unstable_Grid2 as Grid2 } from '@mui/material';
+import { Grid2 } from '@mui/material';

Handled in: #43021

@DiegoAndai
Copy link
Member Author

I'm confused about the names. Are @mui/material/Grid2 and @mui/system/Grid both Grid v2? If yes, then, please, please, let's make the name match. First because the different should only be about either the default theme is preset or not. And second because we are moving to a place where @mui/material/Grid2 and @mui/material/Grid don't exist as exports from this npm package.

Interesting challenge, I didn't look close, I could have seen us go for a straight rename because 1. it's easy to handle for people 2. the API is almost the same, and we can have prop deprecation + props mapping when using the wrong imports to still get most of the behavior right + a clear migration message.

@oliviertassinari, thanks for the feedback. This is the exact debate I went through in my head when deciding not to rename them as it "defeats the purpose of deprecation as it introduces a breaking change (changing the import) instead of offering a deprecation period for users to adjust.". I still think the actual implementation is the correct one, and we're very close to stable, so I don't want to introduce a new change.

As the original Grid is deprecated now, we can plan for it to be removed in v7 and solve this naming confusion once and for all. This is an intermediate step, we'll get there eventually 😊

@oliviertassinari oliviertassinari mentioned this pull request Aug 25, 2024
6 tasks
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 25, 2024

@DiegoAndai Fair points, I guess the main thing I'm trying to get is make progress with #40594 and go to the point where Grid is never exported from @mui/material but only from @mui/system. I could see us supporting @mui/system/Grid2 to ease this transition.

In any cases, issue created: #43437, we need to keep track of this for the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: Grid The React component. package: material-ui Specific to @mui/material package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mui-system][material-ui][Grid v2] Stabilize API
5 participants