[DataGrid] Fix theme scoping issues #10498
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,13 @@ | ||
| import * as React from 'react'; | ||
| import composeClasses from '@mui/utils/composeClasses'; | ||
| import { | ||
| getDataGridUtilityClass, | ||
| gridClasses, | ||
| GridColDef, | ||
| GridColumnHeaderParams, | ||
| GridColumnHeaderTitle, | ||
| } from '@mui/x-data-grid'; | ||
| import { styled, GridBaseColDef } from '@mui/x-data-grid/internals'; | ||
| import { getAggregationFunctionLabel } from '../hooks/features/aggregation/gridAggregationUtils'; | ||
| import { useGridApiContext } from '../hooks/utils/useGridApiContext'; | ||
| import { useGridRootProps } from '../hooks/utils/useGridRootProps'; | ||
|
|
@@ -40,11 +39,15 @@ const GridAggregationFunctionLabel = styled('div', { | |
| overridesResolver: (_, styles) => styles.aggregationColumnHeaderLabel, | ||
| })<{ ownerState: OwnerState }>(({ theme }) => { | ||
| return { | ||
| // @ts-ignore `@mui/material` theme.typography does not exist | ||
| fontSize: theme.typography.caption.fontSize, | ||
| // @ts-ignore `@mui/material` theme.typography does not exist | ||
| lineHeight: theme.typography.caption.fontSize, | ||
| position: 'absolute', | ||
| bottom: 4, | ||
| // @ts-ignore `@mui/material` theme.typography does not exist | ||
| fontWeight: theme.typography.fontWeightMedium, | ||
| // @ts-ignore `@mui/material` theme.vars does not exist | ||
|
Comment on lines
+42
to
+50
There was a problem hiding this comment. I'm also not sure how we should evolve these styles that rely on material theme. Maybe we should define a minimal default grid theme with the styles we need from material? And then we document which theme fields need to be re-defined for other design systems? There was a problem hiding this comment. This sounds like a good approach, I would even recommend introducing CSS vars for these tokens that reads from the theme if the value exists, or fallback to some default value. That way people don’t need to define theme, they can just add CSS vars instead. There was a problem hiding this comment. +1 for creating DataGrid CSS variables. It can be a plain object for type safety, basically to remove accessing the JS theme. Then in the, // dataGridCssVars.ts
// the structure below is just an example.
export const vars = {
typography: {
caption: {
fontSize: '--DataGrid-typography-caption-fontSize',
…
}
}
}
// GridAggregationHeader
fontSize: `var(${vars.typography.caption.fontSize}, 12px)`, // the fallback is optional
// GridRootStyles.ts
const gridStyle: CSSInterpolation = {
[vars.typography.caption.fontSize]: t.typography.caption.fontSize,
...
}There was a problem hiding this comment. Any thoughts on adding those properties on the system theme object instead? One issue I have with CSS variables is that it's going to be a lot of boilerplate to redefine all the theme variables we need. And once we have released publically a set of CSS variables, it gets harder to add new ones because it could be a breaking change if that new variable is not defined by users, whereas the theme object covers basically every theming/styling use-case. If we go with CSS variables, I'm also unsure if/how to adapt patterns like these ones:
There was a problem hiding this comment.
In the case of adding new variables, would we not provide a default in the Data Grid, and then users only need to define it if they wish to override the value? That should prevent breaking changes unless I'm misunderstanding. There was a problem hiding this comment.
What does this mean in practice? We extend the system theme by adding the properties we need, and then each consumer is responsible for providing a compatible theme object, correct? There was a problem hiding this comment.
Yes in theory, in practice I feel like there could be cases where we can't find a good default.
Yes, that would be the idea. The theme object is more structured and already has typings and everything needed for theming. If we go to CSS variables, we'd need to replace pretty much all of these cases with them (unless we only use CSS variables for the cases where the system theme object doesn't match the material theme, but mixing both paradigms sounds bad). There was a problem hiding this comment.
True, that is a valid approach and a practical one.
Moving CSS will take more effort. I think it will involve a design engineer to come up with a minimal set of variables that will be used across the data grid in an efficient way (might include a redesign). But I think it's a long term way because it does not involve JS theme anymore, it's just CSS and anyone can override theme (theming the data grid) with plain CSS. |
||
| color: (theme.vars || theme).palette.primary.dark, | ||
| textTransform: 'uppercase', | ||
| }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the
@ts-ignorewill be removed by my suggestion https://github.com/mui/mui-x/pull/10498/files#r1778208301