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

[DataGrid] Drop v4 support #2515

Merged
merged 8 commits into from
Sep 9, 2021
Merged

[DataGrid] Drop v4 support #2515

merged 8 commits into from
Sep 9, 2021

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Aug 31, 2021

Breaking changes

  • The module augmentation is not enabled by default. This change was done to prevent conflicts with projects using DataGrid and DataGridPro together. In order to still be able to do overrides at the theme level, add the following imports to your project:

    +import type {} from '@mui/x-data-grid/themeAugmentation';
    +import type {} from '@mui/x-data-grid-pro/themeAugmentation';

One of #1902, Preview: https://deploy-preview-2515--material-ui-x.netlify.app/components/data-grid/

The previous module augmentation was using the MuiDataGridPro key for overrides:

https://github.com/mui-org/material-ui-x/blob/afb858cb986c7b86494be7261353fde9fe537b90/packages/grid/x-grid/src/themeAugmentation/props.ts#L4

However, this is not right because even in the Pro version the overrides are retrieved from the MuiDataGrid key, so I updated the definitions to use the same key. As consequence, we can't export these definitions in the index because if a project uses both versions (e.g. @mui/x-data-grid-generator) they will conflict and not compile. That's the reason for the breaking change.

https://github.com/mui-org/material-ui-x/blob/afb858cb986c7b86494be7261353fde9fe537b90/packages/grid/x-grid/src/useDataGridProProps.ts#L11

I didn't update the build config to generate the themeAugmentation folder. I plan to do it in a follow-up.

@m4theushw m4theushw added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Sep 1, 2021
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.

Looks great

packages/grid/data-grid/src/index.ts Show resolved Hide resolved
@@ -21,6 +21,7 @@ const blacklist = [
'docs-components-data-grid-filtering/ColumnTypeFilteringGrid.png', // Needs interaction
'docs-components-data-grid-filtering/CustomRatingOperator.png', // Needs interaction
'docs-components-data-grid-filtering/ExtendNumericOperator.png', // Needs interaction
'docs-components-data-grid-components/CustomFooter.png', // Needs @material-ui/lab v4
Copy link
Member

Choose a reason for hiding this comment

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

Could you detail the issue here?

Copy link
Member Author

@m4theushw m4theushw Sep 3, 2021

Choose a reason for hiding this comment

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

I upgraded all @material-ui/xxx dependencies in the root's package.json to v5, so this regression test will now run with @material-ui/lab v5. However, the Rating component in v5 is not in the lab anymore, then it throws a "The Rating component was moved from the lab to the core" error in the console. Since no test can output to the console it fails.

https://github.com/mui-org/material-ui-x/blob/e214b3a43f639980fde82ff9d62eb40612aa335f/test/regressions/index.test.js#L107

Copy link
Member

Choose a reason for hiding this comment

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

OK, so maybe we should do

Suggested change
'docs-components-data-grid-components/CustomFooter.png', // Needs @material-ui/lab v4
// TODO import the Rating from @mui/material, not the lab.
'docs-components-data-grid-components/CustomFooter.png',

interface ComponentsPropsList extends DataGridProComponentsPropsList {}
}

declare module '@material-ui/core/styles/components' {
Copy link
Member

Choose a reason for hiding this comment

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

It's a private path:

Suggested change
declare module '@material-ui/core/styles/components' {
declare module '@material-ui/core/styles' {

Copy link
Member Author

Choose a reason for hiding this comment

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

The Components interface is not exported. We will be able to remove the private path when @mui/material 5.0.0-rc.0 became the minimum version. The RC includes mui/material-ui#28078.

@flaviendelangle
Copy link
Member

flaviendelangle commented Sep 3, 2021

v4

image

v5

image

It's sad to loose the full label of the header of the column Desk (same on other headers)

Otherwise, the Argos diffs seems logical

@flaviendelangle
Copy link
Member

#2527

@mui-org/x should we switch to the release candidate of the core directly on this PR to avoid having this problem ?
I suppose users already on v5 will quickly upgrade to the release candidate, and thus we should support the new package names rather than the old ones.

@m4theushw
Copy link
Member Author

@flaviendelangle No, this PR is only to remove the v4 dependency. To support the new package name we have to rename all files, so it's better to do in another one.

@m4theushw
Copy link
Member Author

m4theushw commented Sep 3, 2021

It's sad to loose the full label of the header of the column Desk (same on other headers)

@flaviendelangle The IconButton in v5 is 28x28 while it's 24x24 in v4. We can increase the column width in the x-data-grid-generator.

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.

Looks good

@@ -66,7 +66,7 @@ function EditCountry(props: GridRenderEditCellParams) {
open
classes={classes}
disableClearable
renderOption={(option) => (
renderOption={(liProps, option) => (
Copy link
Member

Choose a reason for hiding this comment

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

liProps needs to be applied on a DOM node, to replace the React fragment.

@@ -21,6 +21,7 @@ const blacklist = [
'docs-components-data-grid-filtering/ColumnTypeFilteringGrid.png', // Needs interaction
'docs-components-data-grid-filtering/CustomRatingOperator.png', // Needs interaction
'docs-components-data-grid-filtering/ExtendNumericOperator.png', // Needs interaction
'docs-components-data-grid-components/CustomFooter.png', // Needs @material-ui/lab v4
Copy link
Member

Choose a reason for hiding this comment

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

OK, so maybe we should do

Suggested change
'docs-components-data-grid-components/CustomFooter.png', // Needs @material-ui/lab v4
// TODO import the Rating from @mui/material, not the lab.
'docs-components-data-grid-components/CustomFooter.png',

@flaviendelangle
Copy link
Member

It's sad to loose the full label of the header of the column Desk (same on other headers)

@flaviendelangle The IconButton in v5 is 28x28 while it's 24x24 in v4. We can increase the column width in the x-data-grid-generator.

OK
I think it would be a good idea indeed.

@m4theushw m4theushw merged commit 305affa into mui:next Sep 9, 2021
@m4theushw m4theushw deleted the drop-v4-support branch September 9, 2021 00:21
@m4theushw m4theushw mentioned this pull request Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants