mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.55k stars 1.33k forks source link

[data grid] should work with theme scoping (e.g. theme-ui) #10484

Open jln-dk opened 1 year ago

jln-dk commented 1 year ago

Steps to reproduce

Link to live example: https://codesandbox.io/s/youthful-chaplygin-lw7d8y?file=/src/demo.tsx

Steps:

  1. Create a React app with @mui/material and @mui/x-data-grid.
  2. Do the recommended setup for theme scoping from official MUI docs:
    <ThemeProvider theme={{ [THEME_ID]: materialTheme }}>
    {/* components from another library and Material UI */}
    </ThemeProvider>
  3. Add the <DataGrid> and click on e.g. a checkbox in the data grid.

Current behavior

If you click on e.g. a checkbox in the data grid, the app will break.

Screenshot 2023-09-26 at 16 01 55

Expected behavior

It should work and use the provided theme, just as other components in @mui/material.

Context

With MUI it's possible to do theme scoping to avoid issues when using multiple styling solutions in one app, e.g. theme-ui. See: https://mui.com/material-ui/guides/theme-scoping/#using-with-theme-ui

Example:

import { ThemeUIProvider } from 'theme-ui';
import { createTheme as materialCreateTheme, THEME_ID } from '@mui/material/styles';

const themeUITheme = { ... };
const materialTheme = materialCreateTheme();

function App() {
  return (
    <ThemeUIProvider theme={themeUITheme}>
      <MaterialThemeProvider theme={{ [THEME_ID]: materialTheme }}>
        Theme UI components and Material UI components
      </MaterialThemeProvider>
    </ThemeUIProvider>
  );
}
MBilalShafi commented 1 year ago

Hey @jln-dk, thank you for using MUI X DataGrid and reporting the issue related to theme scoping.

It's the second time in a while we've seen an issue related to the theme scoping, here's the previous one https://github.com/mui/mui-x/issues/10430 (already fixed)

Here's how to fix it:

diff --git a/packages/grid/x-data-grid/src/components/GridSelectedRowCount.tsx b/packages/grid/x-data-grid/src/components/GridSelectedRowCount.tsx
index c389b75a0..81f463f34 100644
--- a/packages/grid/x-data-grid/src/components/GridSelectedRowCount.tsx
+++ b/packages/grid/x-data-grid/src/components/GridSelectedRowCount.tsx
@@ -2,7 +2,7 @@ import * as React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import { unstable_composeClasses as composeClasses } from '@mui/utils';
-import { styled, SxProps, Theme } from '@mui/system';
+import { styled, SxProps, Theme } from '@mui/material/styles';
 import { useGridApiContext } from '../hooks/utils/useGridApiContext';
 import { getDataGridUtilityClass } from '../constants/gridClasses';
 import { useGridRootProps } from '../hooks/utils/useGridRootProps';

Feel free to open up a PR with a fix if you want to see this specific item fixed faster.


For the long term fix: I guess we should update all instances of @mui/system to @mui/material/styles to avoid theme scoping issues in more components. Unless we plan to fix it in @mui/system. πŸ€”

@siriwatknp please let us know if you have some knowledge about any such plans. CC @mui/xgrid

flaviendelangle commented 1 year ago

This would basically undo https://github.com/mui/mui-x/pull/8032 So for me we want to keep @mui/system and make it compatible with theme scoping

yaredtsy commented 1 year ago

It appears that the issue lies with the use of a div element for GridSelectedRowCountRoot, as it is not properly integrated with theme scoping. fix demo: https://codesandbox.io/s/nice-moore-wsp7q8?file=/src/demo.tsx

MBilalShafi commented 1 year ago

I wonder how does it work with the variant from @mui/material/styles then πŸ€”

yaredtsy commented 1 year ago

I wonder how does it work with the variant from @mui/material/styles then πŸ€”

Most of the components use material-ui components, but the ones that are using div and other HTML elements crash.

MBilalShafi commented 1 year ago

If I am getting it right, I think that a proper solution to this problem will be one of the following:

  1. Switch back to the @mui/material/styles (inverse of #8032)
  2. Provide material theme and scoping support on the @mui/system variant styled from the core side.
  3. Have a wrapper around @mui/system styled in the grid repo, include the material theme and use it instead of importing from either of the places (basically replicate part of what's internally being done in @mui/material variant of styled), but it will partially solve the problem (the theme part) as the scoping works based on the THEME_ID exported from @mui/material, so unless we move this id to a common place (like @mui/system for example) and import from there everywhere, it is not likely to work as expected
siriwatknp commented 1 year ago

Let me clarify how the theme scoping works at the moment and we can discuss the way to go for MUI X components.

@mui/system provides a factory function createStyled that can receive themeId to be able to scope the theme if needed.

@mui/material and @mui/joy use createStyled to produce styled API with a unique theme id. The styled is used to build components and also export from the package. All of the theming API will try to get theme.[themeId] if it exists.

The root cause of the error is the styled used by the DataGrid component comes from the system (no themeId), so when theme scoping is created, the DataGrid components still access the root theme which results in an error since all of the data is in theme.[themeId].*.

I think DataGrid should create it's own styled using createStyled from @mui/system with the same theme ID from Material UI (a hard-coded value without importing). This will fix the issue but it won't work with Joy UI in the future.

Here is what it looks like:

// styles/styled.ts
+ import { createStyled } from '@mui/system';

+ // hardcode theme id to avoid importing from @mui/material
+ const styled = createStyled({ themeId: '$$material' });

+ export default styled;

diff --git a/packages/grid/x-data-grid/src/components/GridSelectedRowCount.tsx b/packages/grid/x-data-grid/src/components/GridSelectedRowCount.tsx
index c389b75a0..a0218a542 100644
--- a/packages/grid/x-data-grid/src/components/GridSelectedRowCount.tsx
+++ b/packages/grid/x-data-grid/src/components/GridSelectedRowCount.tsx
@@ -2,7 +2,8 @@ import * as React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import { unstable_composeClasses as composeClasses } from '@mui/utils';
-import { styled, SxProps, Theme } from '@mui/system';
+import { SxProps, Theme } from '@mui/system';
+import styled from '../styles/styled';
 import { useGridApiContext } from '../hooks/utils/useGridApiContext';
 import { getDataGridUtilityClass } from '../constants/gridClasses';
 import { useGridRootProps } from '../hooks/utils/useGridRootProps';
oliviertassinari commented 1 year ago

@siriwatknp's This makes sense in the immediate πŸ‘.

About the long term:

  1. What if the theme scoping was done at the ThemeProvider level? Not in the public API? It could behave as if there were different React contexts. Maybe we didn't do this because of the breaking change impact. People using emotion raw would no longer be able to tap into the Material UI theme.
  2. With @brijeshb42 work on runtime-less CSS-in-JS, I imagine the problem of nesting Joy UI with Material UI is going to be trickier to solve. Theme UI, Charka is pretty much solved, but How would a data grid container know if it needs to use Joy UI spacing of 10px or Material UI spacing of 8px? My best guess is that we would need to duplicate all the components. We would have a <GridSelectedRowCountRoot> for Material UI, and a <GridSelectedRowCountRoot> for Joy UI. In this case, maybe we should already start to move in this direction.
rbrtsmith commented 1 year ago

Hi, is there a workaround for this as an MUI consumer while we wait for a fix to be released?

rbrtsmith commented 11 months ago

Hi @oliviertassinari πŸ‘‹ Do you know if there has been any movement on this? The issue here is blocking us from integrating some components we've made using another library. I'd be happy to raise a PR to implement what @siriwatknp proposes but I'm not sure if that's the direction you want to go in. Could you advise please?

siriwatknp commented 9 months ago

Hi @oliviertassinari πŸ‘‹ Do you know if there has been any movement on this? The issue here is blocking us from integrating some components we've made using another library. I'd be happy to raise a PR to implement what @siriwatknp proposes but I'm not sure if that's the direction you want to go in. Could you advise please?

I added a comment here. You can follow the PR.

jonathantredway commented 6 months ago

Are there any updates coming from this? I see that #10498 hasn't had any commits in 7 months and the last comment was 3 months ago. Has this stalled out?

In the meantime, are there any workarounds that consumers can apply?

jonathantredway commented 6 months ago

I'm also seeing #12432 is open in a draft state and guessing that this relates to this comment. If this is the route, is there an ETA? This is a blocking issue for adopting DataGrid which we would like to do.

romgrk commented 6 months ago

No updates, we plan to remove material-ui bundling but we don't have an ETA and I don't understand the implications for theme scoping which I haven't worked with yet. Maybe @cherniavskii or @MBilalShafi can provide more details on which workarounds could be used.