-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[CssVarsProvider][Material UI] Exclude dark mode variables from :root
stylesheet
#34131
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
49f7263
add excludeVariablesFromRoot
siriwatknp e12dff8
Merge branch 'master' of https://github.com/mui/material-ui into mate…
siriwatknp 764bc4e
add regression
siriwatknp faea52e
add tests
siriwatknp 850c754
update tests
siriwatknp 6ca019c
Merge branch 'master' of https://github.com/mui/material-ui into mate…
siriwatknp 38150cf
flip the logic
siriwatknp d8c321f
fix types
siriwatknp f319aaf
update visual regression
siriwatknp b787863
Merge branch 'master' of https://github.com/mui/material-ui into mate…
siriwatknp f8884d3
fix word
siriwatknp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
41 changes: 41 additions & 0 deletions
41
packages/mui-material/src/styles/excludeVariablesFromRoot.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import { expect } from 'chai'; | ||
import excludeVariablesFromRoot from './excludeVariablesFromRoot'; | ||
|
||
describe('excludeVariablesFromRoot', () => { | ||
it('should return true', () => { | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-1`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-2`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-3`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-4`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-5`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-6`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-7`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-8`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-9`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-10`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-11`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-12`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-13`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-14`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-15`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-16`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-17`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-18`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-19`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-20`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-21`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-22`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-23`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-overlays-24`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-palette-AppBar-darkBg`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('mui').includes(`--mui-palette-AppBar-darkColor`)).to.equal( | ||
true, | ||
); | ||
}); | ||
|
||
it('should return true for custom prefix', () => { | ||
expect(excludeVariablesFromRoot('').includes(`--overlays-1`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('').includes(`--palette-AppBar-darkBg`)).to.equal(true); | ||
expect(excludeVariablesFromRoot('').includes(`--palette-AppBar-darkColor`)).to.equal(true); | ||
}); | ||
}); |
12 changes: 12 additions & 0 deletions
12
packages/mui-material/src/styles/excludeVariablesFromRoot.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/** | ||
* @internal These variables should not appear in the :root stylesheet when the `defaultMode="dark"` | ||
*/ | ||
const excludeVariablesFromRoot = (cssVarPrefix: string) => [ | ||
...[...Array(24)].map( | ||
(_, index) => `--${cssVarPrefix ? `${cssVarPrefix}-` : ''}overlays-${index + 1}`, | ||
), | ||
`--${cssVarPrefix ? `${cssVarPrefix}-` : ''}palette-AppBar-darkBg`, | ||
`--${cssVarPrefix ? `${cssVarPrefix}-` : ''}palette-AppBar-darkColor`, | ||
]; | ||
|
||
export default excludeVariablesFromRoot; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
33 changes: 33 additions & 0 deletions
33
test/regressions/fixtures/CssVarsProvider/MaterialUIDefaultDark.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import * as React from 'react'; | ||
import { Experimental_CssVarsProvider as CssVarsProvider } from '@mui/material/styles'; | ||
import AppBar from '@mui/material/AppBar'; | ||
import Box from '@mui/material/Box'; | ||
import Paper from '@mui/material/Paper'; | ||
import Toolbar from '@mui/material/Toolbar'; | ||
|
||
export default function MaterialUIDefaultDark() { | ||
return ( | ||
<CssVarsProvider defaultMode="dark"> | ||
<Box | ||
sx={{ | ||
display: 'grid', | ||
gridTemplateColumns: 'repeat(auto-fill, minmax(256px, 1fr))', | ||
gridAutoRows: 'minmax(160px, auto)', | ||
gap: 2, | ||
'& > div': { | ||
placeSelf: 'center', | ||
}, | ||
}} | ||
> | ||
<AppBar position="static" color="secondary" elevation={12}> | ||
<Toolbar>The color should be `palette.AppBar.darkBg`</Toolbar> | ||
</AppBar> | ||
<Box sx={{ bgcolor: '#121212', p: 4 }}> | ||
<Paper elevation={24} sx={{ bgcolor: '#121212', p: 2, color: '#fff' }}> | ||
You should see overlay. | ||
</Paper> | ||
</Box> | ||
</Box> | ||
</CssVarsProvider> | ||
); | ||
} |
45 changes: 45 additions & 0 deletions
45
test/regressions/fixtures/CssVarsProvider/MaterialUIDefaultDarkToLight.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import * as React from 'react'; | ||
import { | ||
Experimental_CssVarsProvider as CssVarsProvider, | ||
useColorScheme, | ||
} from '@mui/material/styles'; | ||
import AppBar from '@mui/material/AppBar'; | ||
import Box from '@mui/material/Box'; | ||
import Paper from '@mui/material/Paper'; | ||
import Toolbar from '@mui/material/Toolbar'; | ||
|
||
const LightMode = () => { | ||
const { setMode } = useColorScheme(); | ||
React.useEffect(() => { | ||
setMode('light'); | ||
}, [setMode]); | ||
return null; | ||
}; | ||
|
||
export default function MaterialUIDefaultDark() { | ||
return ( | ||
<CssVarsProvider defaultMode="dark"> | ||
<LightMode /> | ||
<Box | ||
sx={{ | ||
display: 'grid', | ||
gridTemplateColumns: 'repeat(auto-fill, minmax(256px, 1fr))', | ||
gridAutoRows: 'minmax(160px, auto)', | ||
gap: 2, | ||
'& > div': { | ||
placeSelf: 'center', | ||
}, | ||
}} | ||
> | ||
<AppBar position="static" color="secondary" elevation={12}> | ||
<Toolbar>The color should be secondary.</Toolbar> | ||
</AppBar> | ||
<Box sx={{ bgcolor: '#121212', p: 4 }}> | ||
<Paper elevation={24} sx={{ bgcolor: '#121212', p: 2, color: '#fff' }}> | ||
You <em>should not</em> see overlay. | ||
</Paper> | ||
</Box> | ||
</Box> | ||
</CssVarsProvider> | ||
); | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could we add test in the
CssVarsProvider
about this logic? Also, how do we make sure we remove the variables only in light mode? Adding tests for both cases would be great.I would argue that the alternative approach is easier to learn and apply rather than adding new API, and at this moment bundle size doesn't really concerns me that much.
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.
Added 2 regression tests
It might create confusion for developers and be prone to errors:
unknown
variables (do we need to discuss the name?)unknown
variableslight
and `dark.I rather have this done internally, so developers do not need to worry about these edge cases.