[CssBaseline] Use `enableColorScheme` to set `color-scheme` for dark mode #29454

alexfauquette · 2021-11-01T13:26:18Z

I have followed (at least) the PR section of the contributing guide.

This PR use color-scheme to set dark/light mode to the browser such that native components have correct colors (#25016 (comment))

mui-pr-bot · 2021-11-01T13:28:19Z

Details of bundle changes

Generated by 🚫 dangerJS against b863e1d

siriwatknp · 2021-11-02T01:44:26Z

I think it should be a deprecation, not a breaking change.

alexfauquette · 2021-11-02T08:16:06Z

@siriwatknp I imitated the @deprecated comments from what I found in the codebase. Is there something else to do in order to deprecate a function?

mnajdova · 2021-11-02T09:26:16Z

packages/mui-material/src/index.d.ts

@@ -172,6 +172,9 @@ export * from './Container';
 export { default as CssBaseline } from './CssBaseline';
 export * from './CssBaseline';

+/**
+ * @deprecated use color-scheme in CssBaseline or ScopedCssBaseline to modify scrollbar color


Suggested change

* @deprecated use color-scheme in CssBaseline or ScopedCssBaseline to modify scrollbar color

* @deprecated the dark scrollbar is automatically added by the `CSSBaseline` and `ScopeCSSBaseline` using the color-scheme attribute

And seeing this, it is a breaking change. Could we maybe add a prop to enable this? Something like setColorScheme. cc @siriwatknp what do you think?

Could we maybe add a prop to enable this

a prop to which component?

Ah, I got it. a prop to CssBaseline. Yes, that make sense.

<CssBaseline enableColorScheme />

@mnajdova @alexfauquette What do you think?

So if I resume, enableColorScheme is by default set to false such that the behavior does not change when updating the minor version. Dev can set it to true in order to use color-scheme instead of darkScrollbar.

Yes, you are right.

What's the use case for the opt-in prop? Setting the right color sheme by default sounds like a bug fix to me.

If it's about playing it safe because we are unsure, then I would argue for trying out the path that will maximize our learning: be greedy by default, and take one step down if necessary.

Regarding the deprecation, we only deprecated it with TypeScript, not with the runtime, is it expected?

At some point I was wondering about turning the helper to be a generic high level scrollbar color customization tool. But I'm not sure there is a market for it, it might be overkill 😁.

On my side, I have been confused, between the code of the CssBaseline and the code of the website.

The idea was that replacing darkScrollbar with color-scheme could lead to style modification for users. But it is not the case, because darkScrollbar is not set by default in the theme.

So if we set colorScheme: theme.palette.mode as the default behavior of the CssBaseline the following should happen:

For projects using darkScrollbar as proposed in the doc, the scollbar will still be override

For projects not using darkScrollbar scrollbar colors will be override (which is an improvement)

Then, it can be seen as a clean bug fix replacing a hack

siriwatknp · 2021-11-02T10:50:35Z

docs/src/pages/components/css-baseline/css-baseline.md

-### Scrollbars
-
-The colors of the scrollbars can be customized to improve the contrast (especially on Windows). Add this code to your theme (for dark mode).
-
-```jsx
-import darkScrollbar from '@mui/material/darkScrollbar';
-
-const theme = createTheme({
-  components: {
-    MuiCssBaseline: {
-      styleOverrides: {
-        body: theme.palette.mode === 'dark' ? darkScrollbar() : null,
-      },
-    },
-  },
-});
-```
-
-This website uses `darkScrollbar` when dark mode is enabled.
-Be aware, however, that using this utility (and customizing `-webkit-scrollbar`) forces MacOS to always show the scrollbar.
-


I suggest to preserve this section and adding deprecate note after ### Scroll bars and also add a link to color-scheme example.

alexfauquette · 2021-11-02T15:29:25Z

packages/mui-material/src/CssBaseline/CssBaseline.js

  WebkitFontSmoothing: 'antialiased', // Antialiasing.
  MozOsxFontSmoothing: 'grayscale', // Antialiasing.
  // Change from `box-sizing: content-box` so that `width`
  // is not affected by `padding` or `border`.
  boxSizing: 'border-box',
  // Fix font resize problem in iOS
  WebkitTextSizeAdjust: '100%',
-  colorScheme: theme.palette.mode,
+  ...(enableColorScheme && { colorScheme: theme.palette.mode }),
 });


I assume these functions are exported to be used by ScopedCssBaseline, but not used by dev. I do not found any use of them somewhere else. That's why I modified their signature.

alexfauquette · 2021-11-02T15:37:29Z

packages/mui-material/src/ScopedCssBaseline/ScopedCssBaseline.js

-})(({ theme }) => ({
-  ...html,
+})(({ theme, enableColorScheme }) => ({
+  ...html(theme, enableColorScheme),


I do not found how to pass enableColorScheme to this component, and at the same time avoid the following warning

Warning: React does not recognize the enableColorScheme prop on a DOM

index f67c9d661d..c833821012 100644 --- a/packages/mui-material/src/ScopedCssBaseline/ScopedCssBaseline.js +++ b/packages/mui-material/src/ScopedCssBaseline/ScopedCssBaseline.js @@ -34,7 +34,7 @@ const ScopedCssBaselineRoot = styled('div', { const ScopedCssBaseline = React.forwardRef(function ScopedCssBaseline(inProps, ref) { const props = useThemeProps({ props: inProps, name: 'MuiScopedCssBaseline' }); - const { className, component = 'div', ...other } = props; + const { className, component = 'div', enableColorScheme = false, ...other } = props; const ownerState = { ...props,

This should help, we don't want to spread it on the DOM.

And note that you can access it in the styles via ownerProps.enableColorScheme

siriwatknp · 2021-11-03T05:33:04Z

packages/mui-material/src/ScopedCssBaseline/ScopedCssBaseline.d.ts

+    /*
+     * Enable to use color-scheme property to set dark mode
+     */
+    enableColorScheme?: boolean;


Suggested change

/*

* Enable to use color-scheme property to set dark mode

*/

enableColorScheme?: boolean;

/*

* Enable `color-scheme` css property to use `theme.palette.mode`.

* For more details, check out https://developer.mozilla.org/en-US/docs/Web/CSS/color-scheme

* For browser support, check out https://caniuse.com/?search=color-scheme

*/

enableColorScheme?: boolean;

siriwatknp · 2021-11-03T05:34:39Z

packages/mui-material/src/CssBaseline/CssBaseline.js

@@ -24,9 +24,9 @@ export const body = (theme) => ({
  },
 });

-export const styles = (theme) => {
+export const styles = (enableColorScheme) => (theme) => {


Why not this

export const styles = (theme, enableColorScheme) => { }

?
I assume that styles is public?

packages/mui-material/src/CssBaseline/CssBaseline.d.ts

packages/mui-material/src/ScopedCssBaseline/ScopedCssBaseline.d.ts

…d.ts

mnajdova · 2021-11-03T15:09:08Z

packages/mui-material/src/CssBaseline/CssBaseline.js

@@ -54,12 +55,12 @@ export const styles = (theme) => {
 /**
 * Kickstart an elegant, consistent, and simple baseline to build upon.
 */
-function CssBaseline(inProps) {
+function CssBaseline({ enableColorScheme = false, ...inProps }) {


Oops, with the current implementation we would not be able to pull these from the theme's defaultProps. Please destructure this on line 60 after you have the props from the theme as well.

Suggested change

function CssBaseline({ enableColorScheme = false, ...inProps }) {

function CssBaseline(inProps) {

See https://github.com/mui-org/material-ui/pull/29454/files#r742041221

mnajdova · 2021-11-03T15:10:10Z

packages/mui-material/src/CssBaseline/CssBaseline.js

@@ -54,12 +55,12 @@ export const styles = (theme) => {
 /**
 * Kickstart an elegant, consistent, and simple baseline to build upon.
 */
-function CssBaseline(inProps) {
+function CssBaseline({ enableColorScheme = false, ...inProps }) {
  const props = useThemeProps({ props: inProps, name: 'MuiCssBaseline' });
  const { children } = props;


Suggested change

const { children } = props;

const { children, enableColorScheme = false } = props;

mnajdova · 2021-11-03T15:12:39Z

docs/src/modules/components/ThemeContext.js

@@ -229,15 +228,6 @@ export function ThemeProvider(props) {
        spacing,
      },
      dense ? highDensity : null,


Actually, instead of removing this we could do:

{ components: { MuiCssBaseline: { defaultProps: { enableColorScheme: true, }, }, }, },

With this, you shouldn't need the changes in the previous three files. For this to work you need to fix https://github.com/mui-org/material-ui/pull/29454/files#r742040199 :)

siriwatknp · 2021-11-04T08:00:51Z

docs/src/pages/components/css-baseline/css-baseline.md

 Be aware, however, that using this utility (and customizing `-webkit-scrollbar`) forces MacOS to always show the scrollbar.

+This is a deprecated functionality.


https://deploy-preview-29454--material-ui.netlify.app/components/css-baseline/#scrollbars

The text is hard to notice that it is deprecated. I suggest to change to something like this.

### Scrollbars > This API is deprecated, consider using [color-scheme](#color-scheme) instead. The colors of the scrollbars can be customized to improve the contrast (especially on Windows). Add this code to your theme (for dark mode). ### Color scheme This API is introduced in @mui/material (v5.0.7) for switching between "light" and "dark" modes of native components such as scrollbar, using the `color-scheme` css property. To enable it, you can set `enableColorScheme=true` as follow:

mnajdova

Looks great 👌 Now we won't have anymore mismatch in different design systems as we would depend on the native implementation of the browser :)

siriwatknp

👍

alexfauquette added 3 commits November 1, 2021 14:05

remove darkscrollbar

fb80448

adapt ScopedCSSBaseline

366d1dd

update the doc

d9d6202

alexfauquette requested review from m4theushw and mnajdova November 1, 2021 13:29

siriwatknp added the breaking change label Nov 2, 2021

deprecat the darkScrollbar instead of removing it

25e31de

mnajdova reviewed Nov 2, 2021

View reviewed changes

siriwatknp removed the breaking change label Nov 2, 2021

siriwatknp reviewed Nov 2, 2021

View reviewed changes

m4theushw removed their request for review November 2, 2021 11:36

alexfauquette added 2 commits November 2, 2021 16:24

add prop to CssBaseline

4e94c1c

[not working] ScopedCssBaseline

eb2a9b7

alexfauquette commented Nov 2, 2021

View reviewed changes

fix

8f6c3f0

alexfauquette commented Nov 2, 2021

View reviewed changes

siriwatknp reviewed Nov 3, 2021

View reviewed changes

alexfauquette added 4 commits November 3, 2021 10:13

PR feedback on style

d42fe4d

PR feedback on doc

d3248a9

make ScopedCssBaseline works

e910ba2

update css-baseline-doc

c848f84

mnajdova reviewed Nov 3, 2021

View reviewed changes

packages/mui-material/src/CssBaseline/CssBaseline.d.ts Show resolved Hide resolved

Update packages/mui-material/src/CssBaseline/CssBaseline.d.ts

9a1c272

mnajdova reviewed Nov 3, 2021

View reviewed changes

packages/mui-material/src/ScopedCssBaseline/ScopedCssBaseline.d.ts Outdated Show resolved Hide resolved

Update packages/mui-material/src/ScopedCssBaseline/ScopedCssBaseline.…

c2d8a1f

…d.ts

mnajdova reviewed Nov 3, 2021

View reviewed changes

alexfauquette added 2 commits November 3, 2021 16:48

use default props in the doc

0d550be

run scripts

aaed4ad

siriwatknp reviewed Nov 4, 2021

View reviewed changes

doc improvement

b863e1d

mnajdova approved these changes Nov 4, 2021

View reviewed changes

siriwatknp approved these changes Nov 4, 2021

View reviewed changes

alexfauquette merged commit 5b886d5 into mui:master Nov 4, 2021

alexfauquette deleted the use-color-schem branch November 4, 2021 10:38

oliviertassinari changed the title ~~[core] use color-scheme property to deal with dark mode~~ [core] Use color-scheme property to deal with dark mode Nov 4, 2021

mnajdova changed the title ~~[core] Use color-scheme property to deal with dark mode~~ [CSSBaseline] Add enableColorScheme prop so enable using color-scheme property to deal with dark mode Nov 8, 2021

mnajdova mentioned this pull request Nov 10, 2021

[website] Fix dark mode on brand pages #29611

Merged

1 task

oliviertassinari added the component: CssBaseline The React component label Aug 26, 2022

oliviertassinari changed the title ~~[CSSBaseline] Add enableColorScheme prop so enable using color-scheme property to deal with dark mode~~ [CssBaseline] Use enableColorScheme to set color-scheme for dark mode Aug 26, 2022

oliviertassinari changed the title ~~[CssBaseline] Use enableColorScheme to set color-scheme for dark mode~~ [CssBaseline] Use enableColorScheme to set color-scheme for dark mode Aug 26, 2022

This was referenced Aug 26, 2022

[discussion] Preparing v6 #30660

Open

[system] Add enableColorScheme option to getInitColorSchemeScript #33261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CssBaseline] Use `enableColorScheme` to set `color-scheme` for dark mode #29454

[CssBaseline] Use `enableColorScheme` to set `color-scheme` for dark mode #29454

alexfauquette commented Nov 1, 2021

mui-pr-bot commented Nov 1, 2021 •

edited

Loading

siriwatknp commented Nov 2, 2021

alexfauquette commented Nov 2, 2021

mnajdova Nov 2, 2021

siriwatknp Nov 2, 2021

siriwatknp Nov 2, 2021

alexfauquette Nov 2, 2021

siriwatknp Nov 2, 2021

oliviertassinari Nov 4, 2021 •

edited

Loading

oliviertassinari Nov 4, 2021

alexfauquette Nov 10, 2021

siriwatknp Nov 2, 2021

alexfauquette Nov 2, 2021

alexfauquette Nov 2, 2021

mnajdova Nov 3, 2021

mnajdova Nov 3, 2021

siriwatknp Nov 3, 2021

siriwatknp Nov 3, 2021

mnajdova Nov 3, 2021 •

edited

Loading

mnajdova Nov 3, 2021

mnajdova Nov 3, 2021

mnajdova Nov 3, 2021

siriwatknp Nov 4, 2021

mnajdova left a comment

siriwatknp left a comment

	* @deprecated use color-scheme in CssBaseline or ScopedCssBaseline to modify scrollbar color
	* @deprecated the dark scrollbar is automatically added by the `CSSBaseline` and `ScopeCSSBaseline` using the color-scheme attribute

	function CssBaseline({ enableColorScheme = false, ...inProps }) {
	function CssBaseline(inProps) {

	const { children } = props;
	const { children, enableColorScheme = false } = props;

		Be aware, however, that using this utility (and customizing `-webkit-scrollbar`) forces MacOS to always show the scrollbar.

		This is a deprecated functionality.

[CssBaseline] Use enableColorScheme to set color-scheme for dark mode #29454

[CssBaseline] Use enableColorScheme to set color-scheme for dark mode #29454

Conversation

alexfauquette commented Nov 1, 2021

mui-pr-bot commented Nov 1, 2021 • edited Loading

siriwatknp commented Nov 2, 2021

alexfauquette commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

siriwatknp left a comment

Choose a reason for hiding this comment

[CssBaseline] Use `enableColorScheme` to set `color-scheme` for dark mode #29454

[CssBaseline] Use `enableColorScheme` to set `color-scheme` for dark mode #29454

mui-pr-bot commented Nov 1, 2021 •

edited

Loading

oliviertassinari Nov 4, 2021 •

edited

Loading

mnajdova Nov 3, 2021 •

edited

Loading