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

Replace @mui/styles with @mui/system #1234

Closed
2 tasks done
ro7584 opened this issue Mar 15, 2021 · 12 comments · Fixed by #2784
Closed
2 tasks done

Replace @mui/styles with @mui/system #1234

ro7584 opened this issue Mar 15, 2021 · 12 comments · Fixed by #2784
Assignees
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! v5.x

Comments

@ro7584
Copy link

ro7584 commented Mar 15, 2021

Hi ~

I was wondering if it could add @material-ui/styles as dependency for the @material-ui/grid?
The file below was included by @material-ui/data-grid, but missing in package.json.
https://github.com/mui-org/material-ui-x/blob/v4.0.0-alpha.22/packages/grid/_modules_/grid/utils/material-ui-utils.ts#L3

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.
@ro7584 ro7584 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Mar 15, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 16, 2021

@ro7584 Interesting. This import is strictly speaking wrong:

https://github.com/mui-org/material-ui-x/blob/32fbaf095a0cc4391c248253b3a02e354f8f8ff2/packages/grid/_modules_/grid/utils/material-ui-utils.ts#L3

We are not allowed to import from this package. Only from the core based on the current dependencies of the package.json.

As you can see, it's getting resolved in v5 with:

https://github.com/mui-org/material-ui-x/blob/32fbaf095a0cc4391c248253b3a02e354f8f8ff2/packages/grid/_modules_/grid/utils/material-ui-utils.ts#L15-L16

Now, the problem is that @material-ui/styles is a singleton. If we add it as a dependency to the data grid, it could lead to hard to debug CSS rendering issue. I would recommend we simply snooze the issue until we push #441 further (drop v4 support).

Are you facing a specific issue (yarn v2 maybe)?

@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! discussion and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Mar 16, 2021
@oliviertassinari oliviertassinari added this to the v5 milestone Mar 16, 2021
@ro7584
Copy link
Author

ro7584 commented Mar 17, 2021

Are you facing a specific issue (yarn v2 maybe)?

You're right. Recently, I'm trying to use Plug'n'Play and get @material-ui/style resolved error at webpack dev server.

Glad to know that it's getting resolved in v5.
Feel free to snooze this issue. 👍

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed discussion labels Jul 11, 2021
@bclehmann
Copy link

bclehmann commented Jul 14, 2021

It might be worth noting that Material 5 is not actually supported. On NPM it says that you can use DataGrid with Material 5, but this isn't the case.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 14, 2021

@bclehmann It's supported, but there are a few limitations, like this one that can be worked around.

@bclehmann
Copy link

I'm not sure that it makes sense to call it a limitation if merely importing the module will prevent compilation.

I don't think that you can say Material 5 is supported if it requires non-standard (and undocumented) changes to package resolution. And it's probably not supported if the current plan is to not add support until Material 5 leaves beta.

@oliviertassinari
Copy link
Member

@bclehmann Please provide a reproduction, a working demo with core v5: https://codesandbox.io/s/material-demo-forked-93pqu.

@oliviertassinari
Copy link
Member

The idea on this issue is to have @material-ui/data-grid rely on @material-ui/[email protected] (emotion) so we can drop the usage of @material-ui/styles@v4 (JSS). It's about optimizing for v5 users while keeping some degraded support for v4.

@oliviertassinari oliviertassinari modified the milestones: stable release, Sprint 21 Aug 16, 2021
@oliviertassinari oliviertassinari modified the milestones: Sprint 21, Sprint 22 Sep 6, 2021
@m4theushw m4theushw changed the title Missing @material-ui/style as dependencies Replace @mui/styles with @mui/system Sep 20, 2021
@m4theushw
Copy link
Member

I've renamed the title to reuse this issue for the migration to @mui/system. We could investigate how complex would be to lower the specificity with the new API. Related to #99 (comment)

The goal is to be able to override styles with .MuiDataGrid-columnHeader, instead of .MuiDataGrid-root .MuiDataGrid-columnHeader.

https://github.com/mui-org/material-ui-x/blob/68ae5d5d477500ba56564df10a215eb7ad2e22d9/packages/grid/_modules_/grid/components/containers/GridRootStyles.ts#L99

@trickreich
Copy link

I can't folow.. what's status quo? is @mui/styles going to be removed?

@m4theushw
Copy link
Member

I can't folow.. what's status quo? is @mui/styles going to be removed?

@trickreich The @mui/styles package is the JSS-based styling solution used in MUI Core v4, but removed in v5, in place of emotion. Since it's legacy now, users need to have both styling packages to be able to use the DataGrid. Our plan is to remove the legacy package and use emotion only (with support for sx prop), like is done in the core.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 7, 2022

I'm reopening as @mui/styles is still present in the dependencies of this monorepo:
https://github.com/mui-org/material-ui-x/blob/b7fb0169f74cec58817e85ff167d197a244d6652/package.json#L72

And, we even have a demo using it, it's misleading:

https://github.com/mui-org/material-ui-x/blob/6efc5f9f6f49e9a0566e048b2358cf204abc5753/docs/src/pages/components/data-grid/events/CatalogOfEventsNoSnap.js#L13

I couldn't find another GitHub issue about this topic.

@oliviertassinari oliviertassinari added core Infrastructure work going on behind the scenes docs Improvements or additions to the documentation and removed bug 🐛 Something doesn't work breaking change v5.x labels Jan 7, 2022
@oliviertassinari
Copy link
Member

My bad: #2991

@oliviertassinari oliviertassinari added breaking change v5.x and removed docs Improvements or additions to the documentation core Infrastructure work going on behind the scenes labels Jan 7, 2022
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! v5.x
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants