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/
3.91k stars 1.19k forks source link

[DataGrid] GridRootStyles missing columnHeader--withRightBorder and cell--withRightBorder in overridesResolver #9182

Closed zsidnam closed 1 year ago

zsidnam commented 1 year ago

Duplicates

Latest version

Steps to reproduce πŸ•Ή

Link to live example: https://codesandbox.io/s/broken-style-overrides-example-ri10go?file=/demo.tsx

Steps:

  1. Open demo.tsx and observe that the styleOverrides for "cell--withRightBorder" and "columnHeader--withRightBorder" are not being applied, while the styleOverrides for other classes like "columnHeader--sortable" are applied.
  2. Try commenting out styleOverrides.root['.MuiDataGrid-cell--withRightBorder'] and styleOverrides.root['.MuiDataGrid-columnHeader--withRightBorder'] and observe that the border color will go back to the default color (gray) instead of using the colors defined in the non-root style overrides.

Current behavior 😯

When styling DataGrid via the theme, the styles defined in styleOverrides['columnHeader--withRightBorder'] and styleOverrides['cell--withRightBorder'] appear to be ignored instead of getting merged into the default styles for the withRightBorder modifier classes. In order to provide custom styling for these classes, you currently need to target the classes from the root styles.

Expected behavior πŸ€”

The styles defined in styleOverrides['columnHeader--withRightBorder'] and styleOverrides['cell--withRightBorder'] should be merged into the default styles for the withRightBorder modifier classes.

Context πŸ”¦

Our design system is implemented in MUI and we strive to keep as much of the styling as possible in the theme via component styleOverrides. While styling the DataGrid, we noticed that the styles for some of the exposed GridClasses appear to be getting ignored, possibly due to the fact that they are not included in the GridRootStyles overridesResolver. (See here). We currently have to work around this by targeting those classes from within the root styles override.

This inconsistency makes styling from the theme difficult because all of the GridClasses are exposed as keys in the styleOverrides but only some of them actually get merged in, requiring some trial and error on our part to figure out how to override the styles for any given class.

Your environment 🌎

npx @mui/envinfo // npx @mui/envinfo did not work, so I had to gather manually: #### System: - OS: macOs Ventrua 13.3.1 (a) - Node: 16.16.0 - Yarn: 1.22.19 - npm: 8.11.0 #### Browsers: - Chrome: 113.0.5672.126 #### npmPackages: - @emotion/react: 11.11.0 - @emotion/styled: 11.11.0 - @mui/icons-material: 5.11.16 - @mui/material: 5.13.3 - @mui/x-data-grid: 6.5.0 - @mui/x-data-grid-generator: 6.5.0 - @types/react: 18.2.7 - @types/react-dom: 18.2.4 - react: 18.2.0 - react-dom: 18.2.0 - typescript: 5.0.4

Order ID or Support key πŸ’³ (optional)

54437

m4theushw commented 1 year ago

We forgot to apply the styles from these keys. It can be solved with the diff below:

diff --git a/packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts b/packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts
index ef394e1fb..cb9bf5384 100644
--- a/packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts
+++ b/packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts
@@ -50,6 +50,7 @@ export const GridRootStyles = styled('div', {
     { [`& .${gridClasses['cell--rangeBottom']}`]: styles['cell--rangeBottom'] },
     { [`& .${gridClasses['cell--rangeLeft']}`]: styles['cell--rangeLeft'] },
     { [`& .${gridClasses['cell--rangeRight']}`]: styles['cell--rangeRight'] },
+    { [`& .${gridClasses['cell--withRightBorder']}`]: styles['cell--withRightBorder'] },
     { [`& .${gridClasses.cellContent}`]: styles.cellContent },
     { [`& .${gridClasses.cellCheckbox}`]: styles.cellCheckbox },
     { [`& .${gridClasses.cellSkeleton}`]: styles.cellSkeleton },
@@ -62,6 +63,7 @@ export const GridRootStyles = styled('div', {
     { [`& .${gridClasses['columnHeader--numeric']}`]: styles['columnHeader--numeric'] },
     { [`& .${gridClasses['columnHeader--sortable']}`]: styles['columnHeader--sortable'] },
     { [`& .${gridClasses['columnHeader--sorted']}`]: styles['columnHeader--sorted'] },
+    { [`& .${gridClasses['columnHeader--withRightBorder']}`]: styles['columnHeader--withRightBorder'] },
     { [`& .${gridClasses.columnHeader}`]: styles.columnHeader },
     { [`& .${gridClasses.columnHeaderCheckbox}`]: styles.columnHeaderCheckbox },
     { [`& .${gridClasses.columnHeaderDraggableContainer}`]: styles.columnHeaderDraggableContainer },

Do you want to submit a PR?

zsidnam commented 1 year ago

Do you want to submit a PR?

@m4theushw thanks for the quick response! πŸ™ Yes I'd love to submit a PR! I will open one up and link it to this issue πŸ‘

zsidnam commented 1 year ago

@m4theushw it looks like there are quite a few classes in addition to the "--withRightBorder" classes that were not included in the overridesResolver. Is the intention that all gridClasses should be included? I'm curious if it would make sense to iterate over the gridClasses keys instead of adding the keys individually?

type StyleOverrides = Record<string, CSSInterpolation>;

function getGridClassOverrides(styles: StyleOverrides): StyleOverrides[] {
  return Object.keys(gridClasses).reduce<StyleOverrides[]>((acc, className) => {
    acc.push({
      [`&.${className}`]: styles[className],
    });

    return acc;
  }, []);
}

export const GridRootStyles = styled('div', {
  name: 'MuiDataGrid',
  slot: 'Root',
  overridesResolver: (props, styles) => [...getGridClassOverrides(styles), styles.root],
})<{ ownerState: OwnerState }>(({ theme }) => {
m4theushw commented 1 year ago

Is the intention that all gridClasses should be included?

Yes, all classes should be applied in overridesResolver. Since this is a manual process we may forget to add one when a new class gets added.

I'm curious if it would make sense to iterate over the gridClasses keys instead of adding the keys individually?

We don't add new classes too often. I don't think it's worth to iterate over all classes. It may reduce performance when serializing the styles, I don't know. Feel free to submit fixes for other missing classes you encounter.