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.

Sign up for GitHub

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

Jump to bottom

[palette] Normalize the usage of the palette #9970

Merged
merged 1 commit into from
Jan 21, 2018
Merged

[palette] Normalize the usage of the palette #9970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .size-limit
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
{
"name": "The initial cost people pay for using one component",
"path": "test/size/overhead.js",
"limit": "24.9 KB"
"limit": "24.7 KB"
},
{
"name": "The size of the whole library.",
"path": "build/index.js",
"limit": "96.4 KB"
"limit": "96.1 KB"
},
{
"name": "The main bundle of the documentation",
Expand Down
3 changes: 2 additions & 1 deletion docs/src/modules/components/Demo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ const styles = theme => ({
},
},
demo: theme.mixins.gutters({
backgroundColor: theme.palette.background.contentFrame,
backgroundColor:
theme.palette.type === 'light' ? theme.palette.grey[200] : theme.palette.grey[900],
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced theme.palette.grey should exist in the default theme. Two components (Button & ExpansionPanelSummary) are using it for a background color when that should be in theme.palette.background, and one, Switch, should probably be calculating its color some other way (I didn't look further).

In the docs we can just import grey directly.

Copy link
Member Author

@oliviertassinari oliviertassinari Jan 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

grey exists in the default theme because we already import it. It's mainly here for the ease of use. Let's see if we can remove it from the core components to start with :) .

Copy link
Member

@mbrookes mbrookes Jan 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

grey existe in the default theme because we already import it

Sure, but then we also import other colors, and they're not added to the theme.

Let's see if we can remove it from the core components

Sure, but in that case, lets not add it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I won't remove the grey usage. I remember why we inject it now: so people can provide their own tones.

Copy link
Member

Choose a reason for hiding this comment

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

@oliviertassinari. If we fix the couple of components using this to use more appropriate theme keys, users can still change them there.

Don't worry if you don't want to fix it here. I can handle it in a separate PR. One more line to fix in the docs won't make a difference.

Copy link
Member Author

@oliviertassinari oliviertassinari Jan 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If we fix the couple of components using this to use more appropriate theme keys

Same, I have been going the other way around. the palette isn't a hub for all the configuration variables the components might need (we gonna need another solution for this). I think that we should aim for a palette as slim as possible. People shouldn't pay tooltip variable cost when only using a button. Also, it won't scale to the third party components. Code splitting is important. It's also simpler to document.

Copy link
Member

Choose a reason for hiding this comment

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

the palette isn't a hub for all the configuration variables the components might need

Agreed.

(we gonna need another solution for this)

You've already documented a couple. Do we need anything more?

Copy link
Member Author

@oliviertassinari oliviertassinari Jan 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You've already documented a couple. Do we need anything more?

@mbrookes I have been wondering about it since 2016 when @nathanmarks redesigned the styling & theming solution. I don't have the answer 🤔 .

display: 'flex',
justifyContent: 'center',
paddingTop: theme.spacing.unit * 2,
Expand Down
7 changes: 4 additions & 3 deletions docs/src/modules/components/MarkdownElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ const styles = theme => ({
overflow: 'hidden',
},
'& thead': {
fontSize: 12,
fontSize: 13,
fontWeight: theme.typography.fontWeightMedium,
color: theme.palette.text.secondary,
},
Expand All @@ -156,7 +156,7 @@ const styles = theme => ({
color: theme.palette.text.primary,
},
'& td': {
borderBottom: `1px solid ${theme.palette.text.lightDivider}`,
borderBottom: `1px solid ${theme.palette.divider}`,
padding: `${theme.spacing.unit}px ${theme.spacing.unit * 2}px ${theme.spacing.unit}px ${
theme.spacing.unit
}px`,
Expand All @@ -174,7 +174,8 @@ const styles = theme => ({
},
'& th': {
whiteSpace: 'pre',
borderBottom: `1px solid ${theme.palette.text.lightDivider}`,
borderBottom: `1px solid ${theme.palette.divider}`,
fontWeight: theme.typography.fontWeightMedium,
padding: `0 ${theme.spacing.unit * 2}px 0 ${theme.spacing.unit}px`,
textAlign: 'left',
},
Expand Down
3 changes: 2 additions & 1 deletion docs/src/pages/customization/ThemeDefault.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ const styles = theme => ({
root: {
padding: theme.spacing.unit * 2,
paddingTop: 0,
backgroundColor: theme.palette.type === 'light' ? '#fff' : 'rgb(36, 36, 36)',
// Match <Inspector /> default theme.
backgroundColor: theme.palette.type === 'light' ? theme.palette.common.white : '#242424',
minHeight: theme.spacing.unit * 40,
width: '100%',
},
Expand Down
6 changes: 5 additions & 1 deletion docs/src/pages/demos/dividers/ListDividers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,18 @@ function ListDividers(props) {
<ListItem button>
<ListItemText primary="Inbox" />
</ListItem>
<Divider light />
<Divider />
<ListItem button>
<ListItemText primary="Drafts" />
</ListItem>
<Divider />
<ListItem button>
<ListItemText primary="Trash" />
</ListItem>
<Divider light />
<ListItem button>
<ListItemText primary="Spam" />
</ListItem>
</List>
);
}
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/demos/expansion-panels/DetailedExpansionPanel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const styles = theme => ({
flexBasis: '33.3%',
},
helper: {
borderLeft: `2px solid ${theme.palette.text.lightDivider}`,
borderLeft: `2px solid ${theme.palette.divider}`,
padding: `${theme.spacing.unit}px ${theme.spacing.unit * 2}px`,
},
link: {
Expand Down
10 changes: 5 additions & 5 deletions docs/src/pages/demos/menus/ListItemComposition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ const styles = theme => ({
menuItem: {
'&:focus': {
backgroundColor: theme.palette.primary.main,
'& $text, & $icon': {
'& $primary, & $icon': {
color: theme.palette.common.white,
},
},
},
text: {},
primary: {},
icon: {},
});

Expand All @@ -31,19 +31,19 @@ function ListItemComposition(props) {
<ListItemIcon className={classes.icon}>
<SendIcon />
</ListItemIcon>
<ListItemText classes={{ text: classes.text }} inset primary="Sent mail" />
<ListItemText classes={{ primary: classes.primary }} inset primary="Sent mail" />
</MenuItem>
<MenuItem className={classes.menuItem}>
<ListItemIcon className={classes.icon}>
<DraftsIcon />
</ListItemIcon>
<ListItemText classes={{ text: classes.text }} inset primary="Drafts" />
<ListItemText classes={{ primary: classes.primary }} inset primary="Drafts" />
</MenuItem>
<MenuItem className={classes.menuItem}>
<ListItemIcon className={classes.icon}>
<InboxIcon />
</ListItemIcon>
<ListItemText classes={{ text: classes.text }} inset primary="Inbox" />
<ListItemText classes={{ primary: classes.primary }} inset primary="Inbox" />
</MenuItem>
</MenuList>
</Paper>
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/demos/tables/EnhancedTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ EnhancedTableHead.propTypes = {

const toolbarStyles = theme => ({
root: {
paddingRight: 2,
paddingRight: theme.spacing.unit,
},
highlight:
theme.palette.type === 'light'
Expand Down
4 changes: 3 additions & 1 deletion pages/api/button-base.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ filename: /src/ButtonBase/ButtonBase.js

# ButtonBase


`ButtonBase` contains as few styles as possible.
It aims to be a building block for people who want to create a simple button.
It contains a load of style reset and some focus/ripple logic.

## Props

Expand Down
6 changes: 3 additions & 3 deletions pages/api/table-row.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ Any other properties supplied will be [spread to the root element](/guides/api#s
You can override all the class names injected by Material-UI thanks to the `classes` property.
This property accepts the following keys:
- `root`
- `head`
- `footer`
- `hover`
- `typeHead`
- `typeFooter`
- `selected`
- `hover`

Have a look at [overriding with classes](/customization/overrides#overriding-with-classes) section
and the [implementation of the component](https://github.com/mui-org/material-ui/tree/v1-beta/src/Table/TableRow.js)
Expand Down
8 changes: 2 additions & 6 deletions src/Button/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export const styles = theme => ({
},
'&$disabled': {
boxShadow: theme.shadows[0],
backgroundColor: theme.palette.text.divider,
backgroundColor: theme.palette.action.disabledBackground,
},
'&:hover': {
backgroundColor: theme.palette.grey.A100,
Expand All @@ -88,11 +88,7 @@ export const styles = theme => ({
backgroundColor: theme.palette.grey[300],
},
'&$disabled': {
backgroundColor: theme.palette.text.divider,
// Reset on mouse devices
'@media (hover: none)': {
backgroundColor: theme.palette.grey[300],
},
backgroundColor: theme.palette.action.disabledBackground,
},
},
},
Expand Down
5 changes: 5 additions & 0 deletions src/ButtonBase/ButtonBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ export const styles = theme => ({
},
});

/**
* `ButtonBase` contains as few styles as possible.
* It aims to be a building block for people who want to create a simple button.
* It contains a load of style reset and some focus/ripple logic.
*/
class ButtonBase extends React.Component {
state = {
keyboardFocused: false,
Expand Down
5 changes: 3 additions & 2 deletions src/Divider/Divider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import withStyles from '../styles/withStyles';
import { fade } from '../styles/colorManipulator';

export const styles = theme => ({
root: {
Expand All @@ -14,10 +15,10 @@ export const styles = theme => ({
marginLeft: 72,
},
default: {
backgroundColor: theme.palette.text.divider,
backgroundColor: theme.palette.divider,
},
light: {
backgroundColor: theme.palette.text.lightDivider,
backgroundColor: fade(theme.palette.divider, 0.08),
},
absolute: {
position: 'absolute',
Expand Down
8 changes: 4 additions & 4 deletions src/Drawer/Drawer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,16 @@ export const styles = theme => ({
maxHeight: '100vh',
},
paperAnchorDockedLeft: {
borderRight: `1px solid ${theme.palette.text.divider}`,
borderRight: `1px solid ${theme.palette.divider}`,
},
paperAnchorDockedTop: {
borderBottom: `1px solid ${theme.palette.text.divider}`,
borderBottom: `1px solid ${theme.palette.divider}`,
},
paperAnchorDockedRight: {
borderLeft: `1px solid ${theme.palette.text.divider}`,
borderLeft: `1px solid ${theme.palette.divider}`,
},
paperAnchorDockedBottom: {
borderTop: `1px solid ${theme.palette.text.divider}`,
borderTop: `1px solid ${theme.palette.divider}`,
},
modal: {}, // Just here so people can override the style.
});
Expand Down
4 changes: 2 additions & 2 deletions src/ExpansionPanel/ExpansionPanel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const styles = theme => {
height: 1,
content: '""',
opacity: 1,
backgroundColor: theme.palette.text.divider,
backgroundColor: theme.palette.divider,
transition: theme.transitions.create(['opacity', 'background-color'], transition),
},
'&:first-child': {
Expand Down Expand Up @@ -60,7 +60,7 @@ export const styles = theme => {
},
},
disabled: {
backgroundColor: theme.palette.text.divider,
backgroundColor: theme.palette.action.disabledBackground,
},
};
};
Expand Down
2 changes: 0 additions & 2 deletions src/ExpansionPanel/ExpansionPanelSummary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export const styles = theme => {
},
disabled: {
opacity: 0.38,
color: theme.palette.action.disabled,
},
content: {
display: 'flex',
Expand All @@ -48,7 +47,6 @@ export const styles = theme => {
margin: '20px 0',
},
expandIcon: {
color: theme.palette.text.icon,
position: 'absolute',
top: '50%',
right: theme.spacing.unit,
Expand Down
4 changes: 2 additions & 2 deletions src/Form/FormHelperText.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import withStyles from '../styles/withStyles';

export const styles = theme => ({
root: {
color: theme.palette.input.helperText,
color: theme.palette.text.secondary,
fontFamily: theme.typography.fontFamily,
fontSize: theme.typography.pxToRem(12),
textAlign: 'left',
Expand All @@ -21,7 +21,7 @@ export const styles = theme => ({
color: theme.palette.error.main,
},
disabled: {
color: theme.palette.input.disabled,
color: theme.palette.text.disabled,
},
});

Expand Down
39 changes: 18 additions & 21 deletions src/Form/FormLabel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,24 @@ import PropTypes from 'prop-types';
import classNames from 'classnames';
import withStyles from '../styles/withStyles';

export const styles = theme => {
const focusColor = theme.palette.primary[theme.palette.type === 'light' ? 'dark' : 'light'];
return {
root: {
fontFamily: theme.typography.fontFamily,
color: theme.palette.input.labelText,
fontSize: theme.typography.pxToRem(16),
lineHeight: 1,
padding: 0,
},
focused: {
color: focusColor,
},
error: {
color: theme.palette.error.main,
},
disabled: {
color: theme.palette.input.disabled,
},
};
};
export const styles = theme => ({
root: {
fontFamily: theme.typography.fontFamily,
color: theme.palette.text.secondary,
fontSize: theme.typography.pxToRem(16),
lineHeight: 1,
padding: 0,
},
focused: {
color: theme.palette.primary[theme.palette.type === 'light' ? 'dark' : 'light'],
},
error: {
color: theme.palette.error.main,
},
disabled: {
color: theme.palette.text.disabled,
},
});

function FormLabel(props, context) {
const {
Expand Down
16 changes: 8 additions & 8 deletions src/Input/Input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,10 @@ export function isAdornedStart(obj) {
}

export const styles = theme => {
const light = theme.palette.type === 'light';
const placeholder = {
color: 'currentColor',
opacity: theme.palette.type === 'light' ? 0.42 : 0.5,
opacity: light ? 0.42 : 0.5,
transition: theme.transitions.create('opacity', {
duration: theme.transitions.duration.shorter,
easing: theme.transitions.easing.ease,
Expand All @@ -54,8 +55,9 @@ export const styles = theme => {
opacity: 0,
};
const placeholderVisible = {
opacity: theme.palette.type === 'light' ? 0.42 : 0.5,
opacity: light ? 0.42 : 0.5,
};
const bottomLineColor = light ? 'rgba(0, 0, 0, 0.42)' : 'rgba(255, 255, 255, 0.7)';

return {
root: {
Expand All @@ -64,7 +66,7 @@ export const styles = theme => {
alignItems: 'baseline',
position: 'relative',
fontFamily: theme.typography.fontFamily,
color: theme.palette.input.inputText,
color: light ? 'rgba(0, 0, 0, 0.87)' : theme.palette.common.fullWhite,
fontSize: theme.typography.pxToRem(16),
},
formControl: {
Expand All @@ -74,7 +76,7 @@ export const styles = theme => {
},
inkbar: {
'&:after': {
backgroundColor: theme.palette.primary[theme.palette.type === 'light' ? 'dark' : 'light'],
backgroundColor: theme.palette.primary[light ? 'dark' : 'light'],
left: 0,
bottom: 0,
// Doing the other way around crash on IE11 "''" https://github.com/cssinjs/jss/issues/242
Expand Down Expand Up @@ -105,7 +107,7 @@ export const styles = theme => {
},
underline: {
'&:before': {
backgroundColor: theme.palette.input.bottomLine,
backgroundColor: bottomLineColor,
left: 0,
bottom: 0,
// Doing the other way around crash on IE11 "''" https://github.com/cssinjs/jss/issues/242
Expand All @@ -125,9 +127,7 @@ export const styles = theme => {
},
'&$disabled:before': {
background: 'transparent',
backgroundImage: `linear-gradient(to right, ${
theme.palette.input.bottomLine
} 33%, transparent 0%)`,
backgroundImage: `linear-gradient(to right, ${bottomLineColor} 33%, transparent 0%)`,
backgroundPosition: 'left top',
backgroundRepeat: 'repeat-x',
backgroundSize: '5px 1px',
Expand Down
Loading