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] Use styled from system #8032

Merged
merged 14 commits into from
Mar 3, 2023
Merged

[data grid] Use styled from system #8032

merged 14 commits into from
Mar 3, 2023

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Feb 24, 2023

The changes contain 2 patterns:

1. A simple conversion from @mui/material/styles to @mui/system.

These component does not use Material UI theme tokens, so I think it is safe to start using @mui/system.

2. Migrate from Material UI theme token to DataGrid CSS variables. These variables are defined by a single GridRootStyles.

Here is what it looks like when we have Joy UI integrated:

// /material/GridRootStyles.ts
export const GridRootStyles = styled('div', {
  name: 'MuiDataGrid',
  slot: 'Root',
})(({ theme }) => {
  return {
    // we could use --unstable_DataGrid-radius as a prefix at the beginning and keep refining until we are ready to ship CSS variables
    '--DataGrid-radius': theme.shape.borderRadius,
    '--DataGrid-headWeight': theme.typography.fontWeightMedium,
    '--DataGrid-overlayBackground': theme.vars
      ? `rgba(${theme.vars.palette.background.defaultChannel} / ${theme.vars.palette.action.disabledOpacity})`
      : alpha(theme.palette.background.default, theme.palette.action.disabledOpacity),}
})


// /joy/GridRootStyles.ts
export const GridRootStyles = styled('div', {
  name: 'MuiDataGrid',
  slot: 'Root',
})(({ theme }) => {
  return {
    // we could use --unstable_DataGrid-radius as a prefix at the beginning and keep refining until we are ready to ship CSS variables
    '--DataGrid-radius': theme.vars.radius.sm,
    '--DataGrid-headWeight': theme.vars.fontWeight.md,
    '--DataGrid-overlayBackground': `rgba(${theme.vars.palette.neutral.darkChannel} / 0.5)`,}
})

@mui-bot
Copy link

mui-bot commented Feb 24, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8032--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 728.9 1,386.9 728.9 1,003.1 222.196
Sort 100k rows ms 775.8 1,406.3 1,226.8 1,147.98 206.336
Select 100k rows ms 293.8 613.1 335.5 391.8 116.135
Deselect 100k rows ms 181.5 331.3 292.2 258.54 59.639

Generated by 🚫 dangerJS against 9a1af1a

@@ -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/material/styles';
import { styled, SxProps, Theme } from '@mui/system';
Copy link
Member

Choose a reason for hiding this comment

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

Based on mui/material-ui#35840, could we use nested imports here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

styled is exported directly from the index. It does not have a nested folder after the build. We could improve it on the core but it should not block this PR.

Both of these are the same (in terms of efficiency).

import { styled } from '@mui/material/styles';
import { styled } from '@mui/system';

@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Feb 24, 2023
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

🎉

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

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 2, 2023
@siriwatknp
Copy link
Member Author

siriwatknp commented Mar 2, 2023

@cherniavskii I decided to add unstable_ prefix to the new CSS variables to implicitly keep them private at the time being. Once we have Joy UI and are ready to share this with the community, we can remove them.

Note, I think we can just use a simple string for referring to a CSS variable since the syntax --unstable_DataGrid-* is pretty unique.

@siriwatknp siriwatknp changed the title Use styled from system [data grid] Use styled from system Mar 3, 2023
@siriwatknp siriwatknp merged commit 7dd77b7 into next Mar 3, 2023
@siriwatknp siriwatknp deleted the migrate-to-system branch March 3, 2023 06:50
@oliviertassinari oliviertassinari added the design: joy This is about Joy Design, please involve a visual or UX designer in the process label Oct 3, 2023
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! design: joy This is about Joy Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants