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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Paper] Support dark mode brightening based on elevation #25522

Merged
merged 6 commits into from
Mar 31, 2021

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Mar 27, 2021

Breaking changes

  • [Paper] Change the background opacity based on the elevation in dark mode. This change was done to follow the Material Design guidelines. You can revert it in the theme:

    const theme = createTheme({
      components: {
        MuiPaper: {
          styleOverrides: { root: { backgroundImage: 'unset' } },
        },
      },
    });

Closes #18309
Based on #21748
Preview: https://deploy-preview-25522--material-ui.netlify.app/components/paper/
Reference: https://material.io/design/color/dark-theme.html#properties

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 27, 2021

Details of bundle changes

@material-ui/lab: parsed: +0.05% , gzip: +0.09%

Generated by 🚫 dangerJS against fbfd6be

@oliviertassinari oliviertassinari added component: Paper This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process labels Mar 27, 2021
packages/material-ui/src/Paper/Paper.js Outdated Show resolved Hide resolved
packages/material-ui/src/Paper/Paper.js Outdated Show resolved Hide resolved
packages/material-ui/src/Paper/Paper.js Show resolved Hide resolved
@m4theushw m4theushw marked this pull request as ready for review March 29, 2021 21:53
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks almost good

packages/material-ui/src/Paper/Paper.js Outdated Show resolved Hide resolved
@oliviertassinari
Copy link
Member

Josh is probably busy. @m4theushw Do you want move forward? :)

@m4theushw m4theushw merged commit 1f267fa into mui:next Mar 31, 2021
@m4theushw m4theushw deleted the dark-mode-brightening branch March 31, 2021 18:10
Copy link
Member

@joshwooding joshwooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

paper: grey[800],
default: '#303030',
paper: '#121212',
default: '#121212',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the question but is it expected to have such a dark background color as the default on the default dark theme? It doesn't look right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this color comes from the Material Design specification: https://material.io/design/color/dark-theme.html#properties

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree with this general comment, it does look a bit strange. I'm saving this feedback. We have a designer joining the team full-time in the new few weeks.

  • Material Design: #121212 brightness 7%
  • YouTube Music: #040404 brightness 1%
  • YouTube: #131313 brightness 9%
  • Facebook: #131314 brightness 10%
  • Chakra: #141821 brightness 17%
  • HeadlessUI: #0E131D brightness 15%
  • linear.app product: #17181B brightness 14%, linear.app homepage #070707 brightness 2%
  • Twitter: #000000 brightness 0%, Dimmed #111820 16%
  • GitHub: #0C0E12 brightness 9%, Dimmed #1A1D22 18%
  • StackOverflow #222222brightness 18%

It also seems that the documentation should use the default values of the palette.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow this is dark..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also initially shocked at the difference, but I've actually come to quite like the new Material defaults. I assume it also makes extra sense for newer fancy screens which can optimize energy usage for dark pixels.

I imagine it would be useful though (if it doesn't already exist) to be able to specify a base brightness and have the rest of the elevation levels automatically scale, similar to how color palettes can automatically infer dark/light based on tonalOffset.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine it would be useful though (if it doesn't already exist) to be able to specify a base brightness and have the rest of the elevation levels automatically scale, similar to how color palettes can automatically infer dark/light based on tonalOffset.

Done in #25522

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I guess I should have looked at the actual code :P, I see the backgroundImage alphaOverlay now. Awesome stuff!

@Jack-Works
Copy link
Contributor

hello, this is a breaking change visually, it should be documented

@Jack-Works
Copy link
Contributor

I found this new behavior does not look good in our product. Use the following code in the theme to disable it temporally.

    components: {
        MuiPaper: {
            styleOverrides: { root: { backgroundImage: 'unset' } },
        },
    },

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 30, 2021

@m4theushw Do you want to update the migration to the v4 docs page with the proposed workaround? I have updated the PR's description to make it easier for developers.

@Jack-Works
Copy link
Contributor

I think my workaround is not the correct way to recommended to all developers, there should be a new prop to control this behavior so developers can set this prop default to false in the theme to get the old behavior.

@oliviertassinari
Copy link
Member

@Jack-Works This behavior is disabled when the variant is outlined or the elevation is zero. Is this enough?

@Jack-Works
Copy link
Contributor

@Jack-Works This behavior is disabled when the variant is outlined or the elevation is zero. Is this enough?

But what if I want elevation or variant? 🤔

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 30, 2021

@Jack-Works If you want a variant elevation with an elevation and not the background change in dark mode. Do you really want to use the concept of Paper from Material Design? You could do:

<Box sx={{ boxShadow: 3, p: 2, borderRadius: 1 }}>Hello</Box>

Screenshot 2021-04-30 at 11 54 10

@m4theushw What do you think?

@bneigher
Copy link

bneigher commented Apr 30, 2021

I ended up getting dark mode looking as it did before just by changing the default palette base:

import React, { useMemo } from 'react'
import { createMuiTheme, ThemeProvider } from '@material-ui/core'
import { deepPurple, pink, red } from '@material-ui/core/colors'
import useDarkMode from 'use-dark-mode'
import CssBaseline from '@material-ui/core/CssBaseline'

function Theme({ path, children }) {
    const darkMode = useDarkMode()
    const isDark = darkMode.value
    const theme = React.useMemo(() => {
    return createMuiTheme({
      palette: {
        mode: isDark ? 'dark' : 'light',
        primary: PRIMARY,
        secondary: SECONDARY,
        error: red,
        background: {
          paper: isDark ? '#424242' : '#fff',
          default: isDark ? '#303030' : '#fafafa',
        },
      },
    })
  }, [darkMode.value])
  return (
    <ThemeProvider theme={theme}>
      <>
         <CssBaseline />
         {children}
      </>
    </ThemeProvider>
  )
}

export default Theme;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: Paper This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support dark mode brighening based on elevation
8 participants