Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[theme] Improve darkScrollbar helper #25016

Closed
1 task done
Primajin opened this issue Feb 20, 2021 · 12 comments 路 Fixed by #29454
Closed
1 task done

[theme] Improve darkScrollbar helper #25016

Primajin opened this issue Feb 20, 2021 · 12 comments 路 Fixed by #29454
Assignees
Labels
design: material This is about Material Design, please involve a visual or UX designer in the process ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@Primajin
Copy link
Contributor

Primajin commented Feb 20, 2021

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 馃挕

This is just an idea, but I think we should not set the scrollbar colors "arbitrarily" but rather via the theme. That way the colors have a connection to the used theme.

Examples 馃寛

I was trying out something like this: Primajin@adb3890 this for sure needs more tweaking but it should convey the idea.

We use custom scrollbars with Material UI 4 both for dark and light - so for 5 it would be good if the scrollbars respect the theme.

Motivation 馃敠

For example we use a different paper and background color in dark mode than the default theme. The scrollbars should match the paper color (or something from background) so that it fit's with the overall design.

@Primajin Primajin added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 20, 2021
@oliviertassinari oliviertassinari added the design: material This is about Material Design, please involve a visual or UX designer in the process label Feb 20, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 20, 2021

I think that .background.level2, .background.level1 are debt, we should remove them. The documentation using a different theme is a red-flag that the default values are wrong #22112.

On a different note. How about we add a logic to allow computing the other track and active values? It can save time.

diff --git a/packages/material-ui/src/darkScrollbar/index.ts b/packages/material-ui/src/darkScrollbar/index.ts
index af4a3ccc90..72a691c0a1 100644
--- a/packages/material-ui/src/darkScrollbar/index.ts
+++ b/packages/material-ui/src/darkScrollbar/index.ts
@@ -1,11 +1,31 @@
+import { lighten, darken, getLuminance, emphasize } from '@material-ui/core/styles';
+
 // track, thumb and active are derieved from macOS 10.15.7
 const scrollBar = {
-  track: '#2b2b2b',
   thumb: '#6b6b6b',
-  active: '#959595',
 };

-export default function darkScrollbar(options = scrollBar) {
+// Opposite of emphasize
+function diminish(color: string, coefficient = 0.15) {
+  return getLuminance(color) > 0.5 ? lighten(color, coefficient) : darken(color, coefficient);
+}
+
+interface Options {
+  track?: string;
+  thumb: string;
+  active?: string;
+}
+
+export default function darkScrollbar(options: Options = scrollBar) {
+  options = { ...options };
+
+  if (!options.track) {
+    options.track = diminish(options.thumb, 0.6);
+  }
+  if (!options.active) {
+    options.active = emphasize(options.thumb, 0.15);
+  }
+
   return {
     scrollbarColor: `${options.thumb} ${options.track}`,
     '&::-webkit-scrollbar, & *::-webkit-scrollbar': {

I have set the coefficients to reproduce the same outcome as before, using macOS default colors.

@oliviertassinari oliviertassinari changed the title Set scrollbar colors from the theme [theme] Improve darkScrollbar helper Feb 20, 2021
@oliviertassinari oliviertassinari added good first issue Great for first contributions. Enable to learn the contribution process. and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 20, 2021
@Primajin
Copy link
Contributor Author

I also wonder if we should force a macOS~sy kind of style to the scrollbars or go the Github way of just changing the colors without the style of the OS.

Github on MacOS

Screenshot 2021-02-22 at 09 30 17Screenshot 2021-02-22 at 09 30 59

Github on Windows

@oliviertassinari
Copy link
Member

@Primajin The current output of darkScrollbar is not platform-dependent but it would probably be better. Is there a way to detect if we are on Windows/Linux?

@Primajin
Copy link
Contributor Author

in CSS not but in JS one could sniff the navigator or so. But maybe we can get around it by only applying (background-)colors and let the rest handle the OS. So we might want to remove the border radius and things like that.

I couldn't really figure out yet how Github does it here but I'm investigating.
I will play around how it will look like across platforms if we only tinker with the colors and share screenshots.

@Primajin
Copy link
Contributor Author

This one seems quite interesting approach - but maybe there is a shorter way than styling everything:
https://codepen.io/patrikx3/pen/ZEBQQyV

@Primajin
Copy link
Contributor Author

OK I got something working like this:

scrollbars-windows-mac

It has one small sniffer for adding a border-radius when it's on mac, otherwise the browser decides whether to show arrows or not.

What do you think?

@oliviertassinari
Copy link
Member

It looks great 馃檹

@oliviertassinari
Copy link
Member

oliviertassinari commented May 27, 2021

Actually, there is a simpler solution found by @m4theushw:

diff --git a/docs/src/modules/components/ThemeContext.js b/docs/src/modules/components/ThemeContext.js
index 27b703a725..53ca608dc6 100644
--- a/docs/src/modules/components/ThemeContext.js
+++ b/docs/src/modules/components/ThemeContext.js
@@ -9,7 +9,6 @@ import {
 import useMediaQuery from '@material-ui/core/useMediaQuery';
 import { enUS, zhCN, faIR, ruRU, ptBR, esES, frFR, deDE, jaJP } from '@material-ui/core/locale';
 import { blue, pink } from '@material-ui/core/colors';
-import darkScrollbar from '@material-ui/core/darkScrollbar';
 import { unstable_useEnhancedEffect as useEnhancedEffect } from '@material-ui/core/utils';
 import { getCookie } from 'docs/src/modules/utils/helpers';
 import useLazyCSS from 'docs/src/modules/utils/useLazyCSS';
@@ -222,15 +221,6 @@ export function ThemeProvider(props) {
         spacing,
       },
       dense ? highDensity : null,
-      {
-        components: {
-          MuiCssBaseline: {
-            styleOverrides: {
-              body: paletteMode === 'dark' ? darkScrollbar() : null,
-            },
-          },
-        },
-      },
       languageMap[userLanguage],
     );

diff --git a/docs/src/pages/components/css-baseline/css-baseline.md b/docs/src/pages/components/css-baseline/css-baseline.md
index 3f7fb03ee2..0018c8c3da 100644
--- a/docs/src/pages/components/css-baseline/css-baseline.md
+++ b/docs/src/pages/components/css-baseline/css-baseline.md
@@ -67,24 +67,7 @@ The `<html>` and `<body>` elements are updated to provide better page-wide defau

 ### 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 '@material-ui/core/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.
+- `color-scheme` is set globally on the `<html>` to set the current color mode.

 ### Typography

diff --git a/packages/material-ui/src/CssBaseline/CssBaseline.js b/packages/material-ui/src/CssBaseline/CssBaseline.js
index 699b4e2058..4bf1a4e62a 100644
--- a/packages/material-ui/src/CssBaseline/CssBaseline.js
+++ b/packages/material-ui/src/CssBaseline/CssBaseline.js
@@ -3,7 +3,7 @@ import PropTypes from 'prop-types';
 import useThemeProps from '../styles/useThemeProps';
 import GlobalStyles from '../GlobalStyles';

-export const html = {
+export const html = (theme) => ({
   WebkitFontSmoothing: 'antialiased', // Antialiasing.
   MozOsxFontSmoothing: 'grayscale', // Antialiasing.
   // Change from `box-sizing: content-box` so that `width`
@@ -11,7 +11,8 @@ export const html = {
   boxSizing: 'border-box',
   // Fix font resize problem in iOS
   WebkitTextSizeAdjust: '100%',
-};
+  colorScheme: theme.palette.mode,
+});

 export const body = (theme) => ({
   color: theme.palette.text.primary,
@@ -25,7 +26,7 @@ export const body = (theme) => ({

 export const styles = (theme) => {
   let defaultStyles = {
-    html,
+    html: html(theme),
     '*, *::before, *::after': {
       boxSizing: 'inherit',
     },

So we can likely remove darkScrollbar() from the codebase.

@m4theushw
Copy link
Member

m4theushw commented May 27, 2021

@oliviertassinari I didn't think about using the color-scheme CSS property but the meta tag instead. No preference, both seems to work. My only concern is that the stylesheet has to be downloaded first to know which stylesheet the user agent should use. Maybe it can cause some inconsistency when changing between modes, I don't know. The meta tag is recommended to apply the color scheme immediatelly: https://web.dev/color-scheme/#the-color-scheme-meta-tag

@oliviertassinari
Copy link
Member

oliviertassinari commented May 27, 2021

My only concern is that the stylesheet has to be downloaded first to know which stylesheet the user agent should use.

The stylesheet is a blocking resource in our case/React, it's not always true for the web. Nothing is painted before it's loaded. It's either inlined in the <head> in SSR or mounted before rendering in CSR.

@m4theushw
Copy link
Member

From https://html.spec.whatwg.org/multipage/semantics.html#meta-color-scheme:

To aid user agents in rendering the page background with the desired color scheme immediately (rather than waiting for all CSS in the page to load), a 'color-scheme' value can be provided in a meta element.

The page is painted after the stylesheet is loaded, but the background comes first. Maybe we add the meta tag only in the docs to not flicker between the white background and the dark one.

@mnajdova mnajdova added OCD21 and removed good first issue Great for first contributions. Enable to learn the contribution process. labels Jun 30, 2021
@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Jul 16, 2021
@mnajdova mnajdova removed the OCD21 label Aug 24, 2021
@alexfauquette alexfauquette self-assigned this Oct 25, 2021
@Frikki
Copy link

Frikki commented Oct 26, 2021

So what do we do?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design: material This is about Material Design, please involve a visual or UX designer in the process ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants