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 #21748

Closed
wants to merge 3 commits into from

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Jul 11, 2020

Closes #18309

I've added the overlay as a separate class and not as part of elevation because you can't currently have elevation and overlay at the same time unlike in the material spec so you couldn't have the elevation class and an outline.

https://material.io/design/color/dark-theme.html

@joshwooding joshwooding added design: material This is about Material Design, please involve a visual or UX designer in the process component: Paper This is the name of the generic UI component, not the React module! labels Jul 11, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Jul 11, 2020

@material-ui/core: parsed: +0.10% , gzip: +0.10%
@material-ui/lab: parsed: +0.16% , gzip: +0.23%

Details of bundle changes

Generated by 🚫 dangerJS against 80d12c5

@@ -3,6 +3,19 @@ import PropTypes from 'prop-types';
import clsx from 'clsx';
import { chainPropTypes } from '@material-ui/utils';
import withStyles from '../styles/withStyles';
import { fade, useTheme } from '../styles';

// Inspired by https://github.com/material-components/material-components-ios/blob/bca36107405594d5b7b16265a5b0ed698f85a5ee/components/Elevation/src/UIColor%2BMaterialElevation.m#L61
Copy link
Member Author

Choose a reason for hiding this comment

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

It was so hard to find an algorithm for the brightening values...

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if 80% of the value wouldn't come from the dark theme palette of grey colors as opposed to the Paper supporting it (which help too). It could be very interesting to integrate it into the system.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jul 20, 2020
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 15, 2020
@@ -3,6 +3,19 @@ import PropTypes from 'prop-types';
import clsx from 'clsx';
import { chainPropTypes } from '@material-ui/utils';
import withStyles from '../styles/withStyles';
import { fade, useTheme } from '../styles';

// Inspired by https://github.com/material-components/material-components-ios/blob/bca36107405594d5b7b16265a5b0ed698f85a5ee/components/Elevation/src/UIColor%2BMaterialElevation.m#L61
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if 80% of the value wouldn't come from the dark theme palette of grey colors as opposed to the Paper supporting it (which help too). It could be very interesting to integrate it into the system.

Comment on lines +64 to +65
paper: '#121212',
default: '#121212',
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should explore how to integrate this:
https://github.com/mui-org/material-ui/blob/38b259eff17f56d92083058543c29c62ce22d6e3/docs/src/modules/components/ThemeContext.js#L235-L239

Into the library. The fact that we had to extend likely suggest that there is an opportunity.

Choose a reason for hiding this comment

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

Looking into the usage of background level1 and 2 I see it is for hard-coding elevation into the documentation's appBar, drawer etc.
If the color by elevation logic will be implemented the reason for having background levels is not needed because the paper in those components will provide the proper color.
I think providing the elevation colors built in, as a default option at least, will be very comfortable in most use cases.

Copy link
Member

Choose a reason for hiding this comment

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

Looking into the usage of background level1 and 2

We aim to remove the custom theme values of the documentation. It's a signal that the default palette is wrong.

We definitely want to fix #18309

@oliviertassinari
Copy link
Member

I'm closing as the effort has been stale for some time, thanks for the exploration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support dark mode brighening based on elevation
4 participants