-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] Support theme.vars
#6784
Conversation
These are the results for the performance tests:
|
packages/grid/x-data-grid-generator/src/renderer/renderTotalPrice.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-generator/src/renderer/renderTotalPrice.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/components/DataGridProVirtualScroller.tsx
Outdated
Show resolved
Hide resolved
@@ -35,7 +35,9 @@ const GridOverlayRoot = styled('div', { | |||
alignSelf: 'center', | |||
alignItems: 'center', | |||
justifyContent: 'center', | |||
backgroundColor: alpha(theme.palette.background.default, theme.palette.action.disabledOpacity), | |||
backgroundColor: theme.vars | |||
? `rgba(${theme.vars.palette.common.backgroundChannel} / ${theme.vars.palette.action.disabledOpacity})` |
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.
This will yield a different result in dark mode. I think we can add another token for theme.vars.palette.background.defaultChannel
in Material UI.
packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts
Outdated
Show resolved
Hide resolved
I think we can add extra tokens in Material UI for these cases. The downside is the bundle size of the generated HTML (with inline CSS). Ideally, the DataGrid and Pickers should use the existing tokens with the least JS color manipulation. |
For the following part of the code, I realised it comes from the const borderColor =
theme.palette.mode === 'light'
? lighten(alpha(theme.palette.divider, 1), 0.88)
: darken(alpha(theme.palette.divider, 1), 0.68); I also did the following modification -theme.vars.palette.common.backgroundChannel
+theme.vars.palette.background.defaultChannel @siriwatknp do you take care of creating the |
// eslint-disable-next-line no-nested-ternary | ||
const borderColor = theme.vars | ||
? theme.vars.palette.TableCell.border | ||
: theme.palette.mode === 'light' | ||
? lighten(alpha(theme.palette.divider, 1), 0.88) | ||
: darken(alpha(theme.palette.divider, 1), 0.68); |
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.
Nice!
Sure! |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 similar comment
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com> Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
@@ -81,7 +81,7 @@ const GridColumnHeadersPinnedColumnHeaders = styled('div', { | |||
display: 'flex', | |||
flexDirection: 'column', | |||
boxShadow: theme.shadows[2], | |||
backgroundColor: theme.palette.background.default, | |||
backgroundColor: (theme.vars || theme).palette.background.default, | |||
...(theme.palette.mode === 'dark' && { |
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.
Opps! this would not work.
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.
If it can help, getOverlayAlpha(elevation)
is defined as a function, but used with only elevation=2
getOverlayAlpha(2) = 0.07
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.
Found the solution, it can be fixed this way:
There is a variable for the dark mode overlay theme.vars.overlays[2]
. https://github.com/siriwatknp/material-ui/blob/system/use-css-theme-vars/packages/mui-material/src/styles/experimental_extendTheme.js#L75
You can use it like this:
...theme.vars ? {
backgroundImage: theme.vars.overlays[2],
} : {
...theme.palette.mode === 'dark' && {
backgroundImage: ...
}
}
When CSS variables are detected, the styles will be:
...
background-image: var(--mui-overlays-2);
This works fine on both light & dark modes.
@@ -128,7 +128,7 @@ const VirtualScrollerPinnedColumns = styled('div', { | |||
position: 'sticky', | |||
overflow: 'hidden', | |||
zIndex: 1, | |||
backgroundColor: theme.palette.background.default, | |||
backgroundColor: (theme.vars || theme).palette.background.default, | |||
...(theme.palette.mode === 'dark' && { backgroundImage: darkModeBackgroundImage }), |
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.
This too.
@@ -157,7 +157,7 @@ const VirtualScrollerPinnedRows = styled('div', { | |||
position: 'sticky', | |||
// should be above the detail panel | |||
zIndex: 3, | |||
backgroundColor: theme.palette.background.default, | |||
backgroundColor: (theme.vars || theme).palette.background.default, | |||
...(theme.palette.mode === 'dark' && { backgroundImage: darkModeBackgroundImage }), |
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.
This too.
@@ -81,7 +81,7 @@ const GridColumnHeadersPinnedColumnHeaders = styled('div', { | |||
display: 'flex', | |||
flexDirection: 'column', | |||
boxShadow: theme.shadows[2], | |||
backgroundColor: theme.palette.background.default, | |||
backgroundColor: (theme.vars || theme).palette.background.default, | |||
...(theme.palette.mode === 'dark' && { | |||
backgroundImage: `linear-gradient(${alpha('#fff', getOverlayAlpha(2))}, ${alpha( |
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.
It is the same function as https://github.com/siriwatknp/material-ui/blob/system/use-css-theme-vars/packages/mui-material/src/styles/experimental_extendTheme.js#L13, so using theme.vars.overlays[2]
should work the same.
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.
Yes, sounds to already be the case for Paper component
https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Paper/Paper.js#L53-L62
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.
🤩 Awesomeee!
Thanks for all the feedbacks :) |
Fix #6702
Similar to #6778 but more difficult, because we have some customization such as
I do not see how to manage it because
lighten
anddarken
are not CSS functions