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] Agnostic styling #16677

Draft
wants to merge 33 commits into
base: master
Choose a base branch
from
Draft

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Feb 21, 2025

Part of the design-system agnostic work.

New styling solution.

Summary

This PR implements the datagrid styling solution as a new babel plugin that transpiles css() calls to static CSS classnames. The styles are extracted to a separate stylesheet for production mode. In dev mode, the css() calls are injected at runtime. @mui/x-internals/css contains shared logic that is used by the production and development codepaths.

To ensure that classes ordering stays consistent across prod & dev modes, the babel plugin builds an import graph to determine in which order the dev mode css() calls would be evaluated (and we can assume those are always at init time, because we only support static styles). In practice, this means styles defined in leaf imports will be defined after styles in non-leaf imports.

We are abusing a bit the babel build process, as babel plugins are meant to transform one file at a time, not to create new files. We attach a nodejs beforeExit handler to write our final CSS stylesheet. It's not very clean, but it avoids the need to mess further with the build process.

Code transformation examples

For an overview of what is supported, see the test cases: index.test.js

We only support static styles, except for vars.x.y.z, for which we do a bit of hardcoding/magic in order to make it work (the alternative would be manually writing var(--DataGrid-t-x-y-z).

Changes

I've only included the implementation of the babel plugin here, except for GridColumnHeaderTitle which I've used as an example of what would change for the rest of the codebase. I could complete the styling changes as a separate PR to facilitate the review, or as part of this one to ensure everything works before merging.

@romgrk romgrk added the component: data grid This is the name of the generic UI component, not the React module! label Feb 21, 2025
@mui-bot
Copy link

mui-bot commented Feb 23, 2025

Deploy preview: https://deploy-preview-16677--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against e8bf93f

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 23, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Comment on lines +1 to +2
import './index.css';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to decide if we import it directly or not.

Choose a reason for hiding this comment

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

I've tested that it works with Next.js and Vite. But we'll need to test with at least all the popular bundlers to make sure there is no extra configuration needed by users for this to work.

Copy link
Contributor Author

@romgrk romgrk Feb 27, 2025

Choose a reason for hiding this comment

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

We also want it to be insertable as early as possible so that our styles have the least possible specificity (to lose against inline tailwind classes for example). I think users could insert it earlier in their build by manually duplicating import '@mui/x-data-grid/index.css';.

Choose a reason for hiding this comment

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

Yeah. If its only a few CSS files (vs one css file for each js/ts file), then it'd be better to just ask users to import it.

Comment on lines +124 to +131
vm.createContext(context);
vm.runInContext(
`
${state.cssVariables};
__result = eval('(' + __source + ')')
`,
context,
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I run this in an isolated VM because I figured there could be security implications to eval()'ing code submitted by an external contributor, but after thinking about it there isn't more risk than that already present in running package.json scripts. Running vars code in nodejs directly would also pose less limitations and be more performant.

@mnajdova
Copy link
Member

Would it be too much work to set up a repo with the plugin set up so we can test it a bit more end to end?

@romgrk
Copy link
Contributor Author

romgrk commented Feb 25, 2025

Would it be too much work to set up a repo with the plugin set up so we can test it a bit more end to end?

If you comment out the rimraf.sync(...) line in the after() of the test file and run the babel plugin tests (cd packages/babel-plugin-mui-css && pnpm run test), the packages/babel-plugin-css/tests/runtime folder will contain a template that you can copy-paste elsewhere. Run with pnpx babel --verbose --extensions ".js" src --config-file babel-config.json --out-dir output --ignore '*.ts'.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 25, 2025
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 2, 2025
Copy link

github-actions bot commented Mar 2, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants