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

[theme] Improve default primary, secondary and error theme palette #26555

Merged
merged 13 commits into from
Jun 7, 2021
9 changes: 6 additions & 3 deletions docs/src/modules/components/MarkdownElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { makeStyles } from '@material-ui/styles';
import { createTheme } from '@material-ui/core/styles';
import { createTheme, alpha } from '@material-ui/core/styles';

const styles = (theme) => ({
root: {
Expand Down Expand Up @@ -36,7 +36,9 @@ const styles = (theme) => ({
padding: '0 3px',
color: theme.palette.text.primary,
backgroundColor:
theme.palette.mode === 'light' ? 'rgba(255, 229, 100, 0.2)' : 'rgba(255, 229, 100, 0.2)',
theme.palette.mode === 'light'
? 'rgba(255, 229, 100, 0.2)'
: alpha(theme.palette.primary.main, 0.08),
fontSize: '.85em',
borderRadius: 2,
},
Expand Down Expand Up @@ -170,7 +172,8 @@ const styles = (theme) => ({
},
'& a, & a code': {
// Style taken from the Link component
color: theme.palette.primary.main,
color:
theme.palette.mode === 'dark' ? theme.palette.primary.main : theme.palette.primary.dark,
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
textDecoration: 'none',
'&:hover': {
textDecoration: 'underline',
Expand Down
10 changes: 8 additions & 2 deletions packages/material-ui/src/FormLabel/FormLabel.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { expect } from 'chai';
import { createMount, describeConformanceV5, act, createClientRender } from 'test/utils';
import FormLabel, { formLabelClasses as classes } from '@material-ui/core/FormLabel';
import FormControl, { useFormControl } from '@material-ui/core/FormControl';
import { hexToRgb } from '@material-ui/core/styles';
import defaultTheme from '../styles/defaultTheme';
Copy link
Member Author

Choose a reason for hiding this comment

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

use defaultTheme here to remove hard coded color in the test below.

I wonder should defaultTheme be exported as defaultLightTheme from @material-ui/core/styles also?

In my side project I use defaultTheme a lot and it is quite annoying to call createTheme() everytime.


describe('<FormLabel />', () => {
const render = createClientRender();
Expand Down Expand Up @@ -165,15 +167,19 @@ describe('<FormLabel />', () => {
<FormLabel data-testid="FormLabel" color="secondary" focused />,
);
expect(container.querySelector(`.${classes.colorSecondary}`)).to.have.class(classes.focused);
expect(getByTestId('FormLabel')).toHaveComputedStyle({ color: 'rgb(245, 0, 87)' });
expect(getByTestId('FormLabel')).toHaveComputedStyle({
color: hexToRgb(defaultTheme.palette.secondary.main),
});
});

it('should have the error class and style, even when focused', () => {
const { container, getByTestId } = render(
<FormLabel data-testid="FormLabel" color="secondary" focused error />,
);
expect(container.querySelector(`.${classes.colorSecondary}`)).to.have.class(classes.error);
expect(getByTestId('FormLabel')).toHaveComputedStyle({ color: 'rgb(244, 67, 54)' });
expect(getByTestId('FormLabel')).toHaveComputedStyle({
color: hexToRgb(defaultTheme.palette.error.main),
});
});
});
});
74 changes: 54 additions & 20 deletions packages/material-ui/src/styles/createPalette.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { deepmerge } from '@material-ui/utils';
import MuiError from '@material-ui/utils/macros/MuiError.macro';
import common from '../colors/common';
import grey from '../colors/grey';
import indigo from '../colors/indigo';
import pink from '../colors/pink';
import purple from '../colors/purple';
import cyan from '../colors/cyan';
import red from '../colors/red';
import orange from '../colors/orange';
import blue from '../colors/blue';
Expand Down Expand Up @@ -91,32 +91,62 @@ function addLightOrDark(intent, direction, shade, tonalOffset) {
}
}

export default function createPalette(palette) {
const {
primary = {
light: indigo[300],
main: indigo[500],
dark: indigo[700],
},
secondary = {
light: pink.A200,
main: pink.A400,
dark: pink.A700,
},
error = {
light: red[300],
function getDefaultPrimary(mode = 'light') {
if (mode === 'dark') {
return {
main: blue[200],
light: blue[50],
dark: blue[400],
};
}
return {
main: blue[700],
light: blue[400],
dark: blue[800],
};
}

function getDefaultSecondary(mode = 'light') {
if (mode === 'dark') {
return {
main: purple[200],
light: purple[50],
dark: purple[400],
};
}
return {
main: purple[500],
light: purple[300],
dark: purple[700],
};
}

function getDefaultError(mode = 'light') {
if (mode === 'dark') {
return {
main: red[500],
light: red[300],
dark: red[700],
},
};
}
return {
main: red[700],
light: red[400],
dark: red[800],
};
}

export default function createPalette(palette) {
const {
warning = {
light: orange[300],
main: orange[500],
dark: orange[700],
},
info = {
light: blue[300],
main: blue[500],
dark: blue[700],
light: cyan[300],
main: cyan[500],
dark: cyan[700],
},
success = {
light: green[300],
Expand All @@ -129,6 +159,10 @@ export default function createPalette(palette) {
...other
} = palette;

const primary = palette.primary || getDefaultPrimary(mode);
const secondary = palette.secondary || getDefaultSecondary(mode);
const error = palette.error || getDefaultError(mode);

// Use the same logic as
// Bootstrap: https://github.com/twbs/bootstrap/blob/1d6e3710dd447de1a200f29e8fa521f8a0908f70/scss/_functions.scss#L59
// and material-components-web https://github.com/material-components/material-components-web/blob/ac46b8863c4dab9fc22c4c662dc6bd1b65dd652f/packages/mdc-theme/_functions.scss#L54
Expand Down
15 changes: 8 additions & 7 deletions packages/material-ui/src/styles/createPalette.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect } from 'chai';
import { deepOrange, indigo, pink } from '../colors';
import { deepOrange, blue, purple, indigo } from '../colors';
import { darken, lighten } from './colorManipulator';
import createPalette, { dark, light } from './createPalette';

Expand Down Expand Up @@ -85,11 +85,11 @@ describe('createPalette()', () => {

it('should create a dark palette', () => {
const palette = createPalette({ mode: 'dark' });
expect(palette.primary.main, 'should use indigo as the default primary color').to.equal(
indigo[500],
expect(palette.primary.main, 'should use blue as the default primary color').to.equal(
blue[200],
);
expect(palette.secondary.main, 'should use pink as the default secondary color').to.equal(
pink.A400,
expect(palette.secondary.main, 'should use purple as the default secondary color').to.equal(
purple[200],
);
expect(palette.text, 'should use dark theme text').to.equal(dark.text);
});
Expand Down Expand Up @@ -176,8 +176,9 @@ describe('createPalette()', () => {
contrastThreshold: 0,
}));
}).toErrorDev([
'falls below the WCAG recommended absolute minimum contrast ratio of 3:1',
'falls below the WCAG recommended absolute minimum contrast ratio of 3:1',
'falls below the WCAG recommended absolute minimum contrast ratio of 3:1', // warning palette
'falls below the WCAG recommended absolute minimum contrast ratio of 3:1', // info palette
'falls below the WCAG recommended absolute minimum contrast ratio of 3:1', // success palette
Comment on lines +179 to +181
Copy link
Member

Choose a reason for hiding this comment

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

Do we include the color in the warning? If not then we can follow-up. Otherwise I'd make sure the color is included in the assertion which means we can get rid of the (potentially wrong) code comments.

Copy link
Member Author

@siriwatknp siriwatknp Jun 3, 2021

Choose a reason for hiding this comment

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

2 lines was there at the beginning (1 more is added because info is changed to cyan[500], it's sound intentional that we expect the colors to falls below contrast ratio. I was confused at first about this test so I added comments at the end. Any suggestion?

]);

expect(() => {
Expand Down