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

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

Open
jln-dk opened this issue Sep 26, 2023 · 14 comments · May be fixed by #10498
Open

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

jln-dk opened this issue Sep 26, 2023 · 14 comments · May be fixed by #10498
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer good first issue Great for first contributions. Enable to learn the contribution process. package: system Specific to @mui/system

Comments

@jln-dk
Copy link

jln-dk commented Sep 26, 2023

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>
  1. 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>
  );
}
@jln-dk jln-dk added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 26, 2023
@MBilalShafi MBilalShafi added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. design This is about UI or UX design, please involve a designer and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 26, 2023
@MBilalShafi
Copy link
Member

MBilalShafi commented Sep 26, 2023

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 #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
Copy link
Member

flaviendelangle commented Sep 26, 2023

This would basically undo #8032
So for me we want to keep @mui/system and make it compatible with theme scoping

@yaredtsy yaredtsy linked a pull request Sep 27, 2023 that will close this issue
1 task
@yaredtsy
Copy link
Contributor

yaredtsy commented Sep 27, 2023

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
Copy link
Member

I wonder how does it work with the variant from @mui/material/styles then 🤔

@yaredtsy
Copy link
Contributor

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
Copy link
Member

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 [data grid] Use styled from system #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
Copy link
Member

siriwatknp commented Oct 3, 2023

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
Copy link
Member

oliviertassinari commented Oct 3, 2023

@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
Copy link

rbrtsmith commented Nov 6, 2023

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

@rbrtsmith
Copy link

rbrtsmith commented Dec 18, 2023

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
Copy link
Member

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
Copy link

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
Copy link

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
Copy link
Contributor

romgrk commented May 15, 2024

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.

@oliviertassinari oliviertassinari changed the title DataGrid should work with theme scoping (e.g. theme-ui) [data grid] should work with theme scoping (e.g. theme-ui) Sep 1, 2024
@oliviertassinari oliviertassinari added the package: system Specific to @mui/system label Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer good first issue Great for first contributions. Enable to learn the contribution process. package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants