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

[Button] Custom variant #21648

Merged
merged 86 commits into from
Jul 27, 2020
Merged
Show file tree
Hide file tree
Changes from 73 commits
Commits
Show all changes
86 commits
Select commit Hold shift + click to select a range
97aac4e
wip
mnajdova Jun 29, 2020
dd57f8e
wip
mnajdova Jun 29, 2020
5c3d592
wip
mnajdova Jun 30, 2020
8ba964e
relaxed overrides
mnajdova Jul 1, 2020
6e0ab4d
added additions
mnajdova Jul 2, 2020
5c8364c
cleanup
mnajdova Jul 2, 2020
653c1e7
cleanup
mnajdova Jul 2, 2020
1755be6
prettier
mnajdova Jul 2, 2020
8c0ae69
wip
mnajdova Jul 4, 2020
036a172
another alternative
mnajdova Jul 5, 2020
1b94f50
Update packages/material-ui-styles/src/getStylesCreator/getStylesCrea…
mnajdova Jul 5, 2020
9370c1c
comb of props
mnajdova Jul 5, 2020
df60098
Update packages/material-ui-styles/src/getStylesCreator/getStylesCrea…
mnajdova Jul 5, 2020
f430770
cleanup
mnajdova Jul 6, 2020
2375c98
ts example updated
mnajdova Jul 6, 2020
7db6a98
Merge branch 'next' into feat/custom-variants
mnajdova Jul 6, 2020
c032672
Update packages/material-ui-styles/src/withStyles/withStyles.js
mnajdova Jul 6, 2020
22d5f4a
Update packages/material-ui-styles/src/withStyles/withStyles.js
mnajdova Jul 6, 2020
3fcb966
reverted changes
mnajdova Jul 6, 2020
212db32
Update packages/material-ui/src/styles/createMuiTheme.js
mnajdova Jul 6, 2020
f92dbaa
tests fixes
mnajdova Jul 6, 2020
ce2369f
prettier
mnajdova Jul 6, 2020
0e67339
addressing comments
mnajdova Jul 7, 2020
ccaf13f
prettier
mnajdova Jul 7, 2020
070ffe0
replace relaxed typings with ts declare module
mnajdova Jul 7, 2020
a86137e
improved description
mnajdova Jul 7, 2020
628e8fd
themeMerge
mnajdova Jul 8, 2020
132c930
prettier
mnajdova Jul 8, 2020
5036d2d
extract capitalize in utils
mnajdova Jul 10, 2020
5e79126
removed mergeThemes
mnajdova Jul 10, 2020
fa883ea
fix
mnajdova Jul 10, 2020
0ab15a6
fixes
mnajdova Jul 10, 2020
fed904b
break circular dependency
mnajdova Jul 10, 2020
9fcc431
restricted to variables
mnajdova Jul 10, 2020
313f8bf
reverted some changes
mnajdova Jul 10, 2020
60787ab
docs:api
mnajdova Jul 10, 2020
f04ccb4
removed new capitalize method
mnajdova Jul 10, 2020
1b81481
removed default exprot
mnajdova Jul 11, 2020
7f12e68
revert some changes, improved example
mnajdova Jul 11, 2020
e1c2d6d
prettier
mnajdova Jul 11, 2020
4f34120
example updated
mnajdova Jul 12, 2020
4f0a648
Update packages/material-ui-styles/src/getStylesCreator/getStylesCrea…
mnajdova Jul 18, 2020
beab317
updated demo
mnajdova Jul 18, 2020
0c00ea9
Merge branch 'feat/custom-variants' of https://github.com/mnajdova/ma…
mnajdova Jul 18, 2020
70b713d
Merge branch 'next' into feat/custom-variants
mnajdova Jul 18, 2020
bb687a4
prettier
mnajdova Jul 18, 2020
4efaf62
lint
mnajdova Jul 18, 2020
18de558
Update packages/material-ui-styles/src/getStylesCreator/getStylesCrea…
mnajdova Jul 19, 2020
4cb0b03
Update packages/material-ui-styles/src/getStylesCreator/getStylesCrea…
mnajdova Jul 19, 2020
e780712
Update docs/src/pages/customization/components/components.md
mnajdova Jul 19, 2020
eb02491
added test
mnajdova Jul 19, 2020
bb5baa1
prettier
mnajdova Jul 19, 2020
3e1ea72
Update docs/src/pages/customization/components/components.md
mnajdova Jul 19, 2020
90609ff
Update docs/src/pages/customization/components/components.md
mnajdova Jul 19, 2020
c4df879
added warning, renamed matcher to props
mnajdova Jul 20, 2020
7d4fa1d
tests
mnajdova Jul 21, 2020
7ae76f7
tests fixes
mnajdova Jul 21, 2020
c0a9d55
prettier
mnajdova Jul 21, 2020
f888036
prettier
mnajdova Jul 21, 2020
b26d20d
added logic for dynamic classkeys generation
mnajdova Jul 23, 2020
75cd3b6
Merge branch 'next' into feat/custom-variants
mnajdova Jul 23, 2020
5365b71
merge conflicts
mnajdova Jul 23, 2020
2916cd6
added tests for warnings, refactored tests
mnajdova Jul 23, 2020
e185968
prettier
mnajdova Jul 23, 2020
2b32fe8
improved test
mnajdova Jul 23, 2020
efe2e7d
Update packages/material-ui-styles/src/makeStyles/makeStyles.js
mnajdova Jul 23, 2020
4ce2381
Update packages/material-ui-styles/src/makeStyles/makeStyles.js
mnajdova Jul 23, 2020
048cd39
Update packages/material-ui/src/Button/Button.test.js
mnajdova Jul 23, 2020
57e4700
addressing comments
mnajdova Jul 23, 2020
4196182
added more description to the docs section
mnajdova Jul 23, 2020
515933e
prettier
mnajdova Jul 23, 2020
1d240c0
added test
mnajdova Jul 23, 2020
b9ac3ca
fixed name
mnajdova Jul 23, 2020
af088c9
polish
oliviertassinari Jul 23, 2020
4e8de25
added hook for variants
mnajdova Jul 24, 2020
20477c6
prettier
mnajdova Jul 24, 2020
0aa43ad
added test
mnajdova Jul 24, 2020
c71e4ba
prettier
mnajdova Jul 24, 2020
2bb3a29
Update packages/material-ui-styles/src/useThemeVariants/useThemeVaria…
mnajdova Jul 27, 2020
52d11c8
Update packages/material-ui/src/Button/Button.js
mnajdova Jul 27, 2020
55f0507
Update packages/material-ui-styles/src/useThemeVariants/useThemeVaria…
mnajdova Jul 27, 2020
44e2f1d
Update packages/material-ui/src/styles/createMuiTheme.js
mnajdova Jul 27, 2020
a55dfe4
Update packages/material-ui-styles/src/useThemeVariants/useThemeVaria…
mnajdova Jul 27, 2020
6b1742a
Update packages/material-ui/src/styles/createMuiTheme.js
mnajdova Jul 27, 2020
bcae6ef
fixed props destructuring order
mnajdova Jul 27, 2020
704e2f2
Merge branch 'next' into feat/custom-variants
mnajdova Jul 27, 2020
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
2 changes: 1 addition & 1 deletion docs/pages/api-docs/button.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ The `MuiButton` name can be used for providing [default props](/customization/gl
| <span class="prop-name">href</span> | <span class="prop-type">string</span> | | The URL to link to when the button is clicked. If defined, an `a` element will be used as the root node. |
| <span class="prop-name">size</span> | <span class="prop-type">'large'<br>&#124;&nbsp;'medium'<br>&#124;&nbsp;'small'</span> | <span class="prop-default">'medium'</span> | The size of the button. `small` is equivalent to the dense button styling. |
| <span class="prop-name">startIcon</span> | <span class="prop-type">node</span> | | Element placed before the children. |
| <span class="prop-name">variant</span> | <span class="prop-type">'contained'<br>&#124;&nbsp;'outlined'<br>&#124;&nbsp;'text'</span> | <span class="prop-default">'text'</span> | The variant to use. |
| <span class="prop-name">variant</span> | <span class="prop-type">'contained'<br>&#124;&nbsp;'outlined'<br>&#124;&nbsp;'text'<br>&#124;&nbsp;string</span> | <span class="prop-default">'text'</span> | The variant to use. |
Copy link
Member

Choose a reason for hiding this comment

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

string shouldn't be allowed by default. Will take a look later. Nothing that holds back this PR since it would also affect breakpoints.


The `ref` is forwarded to the root element.

Expand Down
73 changes: 73 additions & 0 deletions docs/src/pages/customization/components/GlobalThemeVariants.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import React from 'react';
import {
createMuiTheme,
makeStyles,
ThemeProvider,
} from '@material-ui/core/styles';
import Button from '@material-ui/core/Button';

const useStyles = makeStyles((theme) => ({
root: {
'& > *': {
margin: theme.spacing(1),
},
},
}));

const defaultTheme = createMuiTheme();

const inputTheme = createMuiTheme({
variants: {
MuiButton: [
{
props: { variant: 'dashed' },
styles: {
padding: '5px 15px',
border: `3px dashed ${defaultTheme.palette.primary.main}`,
},
},
{
props: { variant: 'dashed', color: 'secondary' },
styles: {
border: `3px dashed ${defaultTheme.palette.secondary.main}`,
},
},
{
props: { variant: 'dashed', size: 'large' },
styles: {
borderWidth: 5,
},
},
{
props: { variant: 'dashed', color: 'primary', size: 'large' },
styles: {
fontWeight: 600,
},
},
],
},
});

export default function GlobalThemeVariants() {
const classes = useStyles();

return (
<div className={classes.root}>
<ThemeProvider theme={inputTheme}>
<Button variant="dashed">Default</Button>
<Button variant="dashed" color="secondary">
Secondary
</Button>
<Button variant="dashed" color="primary">
Primary
</Button>
<Button variant="dashed" color="secondary" size="large">
Secondary large
</Button>
<Button variant="dashed" color="primary" size="large">
Primary large
</Button>
</ThemeProvider>
</div>
);
}
80 changes: 80 additions & 0 deletions docs/src/pages/customization/components/GlobalThemeVariants.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import React from 'react';
import {
createMuiTheme,
makeStyles,
Theme,
ThemeProvider,
} from '@material-ui/core/styles';
import Button from '@material-ui/core/Button';

declare module '@material-ui/core/Button/Button' {
interface ButtonPropsVariantOverrides {
dashed: true;
}
}

const useStyles = makeStyles((theme: Theme) => ({
root: {
'& > *': {
Copy link
Member

@oliviertassinari oliviertassinari Jul 9, 2020

Choose a reason for hiding this comment

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

Mentioning this for later, I have seen a growing trend toward the Stack component approach in the community. I have been benchmarking on https://trello.com/c/vlLBgJca/2404-stack-component. I think that it would be great to introduce such a component in the future. It will allow us to simplify our demos.

margin: theme.spacing(1),
},
},
}));

const defaultTheme = createMuiTheme();

const inputTheme = createMuiTheme({
variants: {
MuiButton: [
{
props: { variant: 'dashed' },
styles: {
padding: '5px 15px',
border: `3px dashed ${defaultTheme.palette.primary.main}`,
},
},
{
props: { variant: 'dashed', color: 'secondary' },
styles: {
border: `3px dashed ${defaultTheme.palette.secondary.main}`,
},
},
{
props: { variant: 'dashed', size: 'large' },
styles: {
borderWidth: 5,
},
},
{
props: { variant: 'dashed', color: 'primary', size: 'large' },
styles: {
fontWeight: 600,
},
},
],
},
});

export default function GlobalThemeVariants() {
const classes = useStyles();

return (
<div className={classes.root}>
<ThemeProvider theme={inputTheme}>
<Button variant="dashed">Default</Button>
<Button variant="dashed" color="secondary">
Secondary
</Button>
<Button variant="dashed" color="primary">
Primary
</Button>
<Button variant="dashed" color="secondary" size="large">
Secondary large
</Button>
<Button variant="dashed" color="primary" size="large">
Primary large
</Button>
</ThemeProvider>
</div>
);
}
40 changes: 40 additions & 0 deletions docs/src/pages/customization/components/components.md
Original file line number Diff line number Diff line change
Expand Up @@ -319,3 +319,43 @@ const theme = createMuiTheme({
```

{{"demo": "pages/customization/components/GlobalThemeOverride.js"}}

### Adding new component variants

You can take advantage of the `variants` key of the `theme` to add new variants to Material-UI components. These new variants, can specify which styles the component should have, if specific properties are defined together.

The definitions are specified in an array, under the component's name. For every one of them a class is added in the head. The order is **important**, so make sure that the styles that should win will be specified lastly.

```jsx
const theme = createMuiTheme({
variants: {
MuiButton: [
{
props: { variant: 'dashed' },
styles: {
padding: '5px 15px',
border: `5px dashed grey${blue[500]}`,
},
},
{
props: { variant: 'dashed', color: 'secondary' },
styles: {
border: `5px dashed ${red[500]}`,
},
},
],
},
});
```

If you are using TypeScript, you will need to specify your new variants/colors, using module augmentation.

```tsx
declare module '@material-ui/core/Button/Button' {
interface ButtonPropsVariantOverrides {
dashed: true;
}
}
```

{{"demo": "pages/customization/components/GlobalThemeVariants.js"}}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { deepmerge } from '@material-ui/utils';
import propsToClassKey from '../propsToClassKey';
import noopTheme from './noopTheme';

export default function getStylesCreator(stylesOrCreator) {
Expand Down Expand Up @@ -36,11 +37,15 @@ export default function getStylesCreator(stylesOrCreator) {
throw err;
}

if (!name || !theme.overrides || !theme.overrides[name]) {
if (
!name ||
((!theme.overrides || !theme.overrides[name]) && (!theme.variants || !theme.variants[name]))
) {
return styles;
}

const overrides = theme.overrides[name];
const overrides = (theme.overrides && theme.overrides[name]) || {};
const variants = (theme.variants && theme.variants[name]) || [];
const stylesWithOverrides = { ...styles };

Object.keys(overrides).forEach((key) => {
Expand All @@ -50,12 +55,22 @@ export default function getStylesCreator(stylesOrCreator) {
[
'Material-UI: You are trying to override a style that does not exist.',
`Fix the \`${key}\` key of \`theme.overrides.${name}\`.`,
'',
'If you intentionally wanted to add a new key, please use the theme.variants option.',
].join('\n'),
);
}
}

stylesWithOverrides[key] = deepmerge(stylesWithOverrides[key], overrides[key]);
stylesWithOverrides[key] = deepmerge(stylesWithOverrides[key] || {}, overrides[key]);
});

variants.forEach((definition) => {
const classKey = propsToClassKey(definition.props);
stylesWithOverrides[classKey] = deepmerge(
stylesWithOverrides[classKey] || {},
definition.styles,
);
});

return stylesWithOverrides;
Expand Down
17 changes: 17 additions & 0 deletions packages/material-ui-styles/src/makeStyles/makeStyles.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,23 @@ export default function makeStyles(stylesOrCreator, options = {}) {
// eslint-disable-next-line react-hooks/rules-of-hooks
React.useDebugValue(classes);
}
if (process.env.NODE_ENV !== 'production') {
const whitelistedComponents = ['MuiButton'];
Copy link
Member

Choose a reason for hiding this comment

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

If we want to whitelist then this should be used throughout the codebase. Otherwise this will grow stale.
Why don't we allow it for e.g. TextField?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not all components follow the logic of having the variant be used as classes[variant], the Typography and some other components are examples of this. So I wanted to avoid wrong warnings here, by whitelisting which components have custom variants enabled so far. Is there a better place where we can define this?

Copy link
Member

Choose a reason for hiding this comment

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

So even though both components have a variant prop I couldn't use this API to add another Typography variant? The problem is that one has to be aware of the implementation and technical limitations of the new API. I find this very problematic. Do you have a plan to make it work for Typography as well? Doesn't have to be in this PR (as a MVP) but if we won't be able to reconcile this difference then this API will cause quite a bit of confusion.

At least we should have a descriptive warning if one wants to extend variants in components where this isn't supported.

Copy link
Member Author

@mnajdova mnajdova Jul 24, 2020

Choose a reason for hiding this comment

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

Even now, you can add custom variants to the Typography, because the newly created classes from the variants key are self generated and used in the same manner. The only thing we need to add per component is new type for allowing users to extend the variant TS type, as well as relax the propTypes for it. Then we can work on the warnings per component if we don't want to introduce breaking changes on how the classes have been generated so far

Tested locally with this example

const theme = createMuiTheme({
  variants: {
    MuiTypography: [
      {
        props: { variant: 'custom' },
        styles: { backgroundColor: 'green' }
      }
    ]
  },
});

// usage
<Typography variant="custom">Custom</Typography>

image


Update: aftre we introduced the hook for the custom variants, the hook will also need to be added in the components

Copy link
Member

Choose a reason for hiding this comment

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

So you're only adding the whitelist because propTypes and TS would warn about it? But right now the warning is saying that it isn't supported. propTypes and TS are just an interface description. If we agree that this is the correct interface then we don't need this extra allow-list. Otherwise we have to keep track of propTypes/TS in this allow-list which just adds maintenance burden without any benefit as far as I can tell.

Copy link
Member Author

Choose a reason for hiding this comment

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

For implementation of the feature yes, proptypes, TS and the new hook should be added.

I was trying to keep the warning as simple as possible, that's why it is checking only if the classes[variant] exists. Once we enable it per component we can see if we need to adjust it..

We can complicate more by checking the theme's variants props and try to find a matcher there, or see if the component itself is handling that variant, but I didn't wanted to complicate until it really is necessary.

The Typography for example doesn't have classes[variant] for the value inherit so this will always throw. That's why I think we will need to anyway adjusts the warning when adding new components.

Let me know if this makes sense.


if (
name &&
whitelistedComponents.indexOf(name) >= 0 &&
props.variant &&
!classes[props.variant]
) {
console.error(
[
`Material-UI: You are using a variant value \`${props.variant}\` for which you didn't define styles.`,
`Please create a new variant matcher in your theme for this variant. To learn more about matchers visit https://material-ui.com/customization/components/#adding-new-component-variants.`,
].join('\n'),
);
}
}

return classes;
};
Expand Down
2 changes: 2 additions & 0 deletions packages/material-ui-styles/src/propsToClassKey/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { default } from './propsToClassKey';
export * from './propsToClassKey';
1 change: 1 addition & 0 deletions packages/material-ui-styles/src/propsToClassKey/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './propsToClassKey';
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default function propsToClassKey(props: object): string;
38 changes: 38 additions & 0 deletions packages/material-ui-styles/src/propsToClassKey/propsToClassKey.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import MuiError from '@material-ui/utils/macros/MuiError.macro';

// TODO: remove this once the capitalize method is moved to the @material-ui/utils package
export function capitalize(string) {
if (typeof string !== 'string') {
throw new MuiError('Material-UI: capitalize(string) expects a string argument.');
}

return string.charAt(0).toUpperCase() + string.slice(1);
}

function isEmpty(string) {
return string.length === 0;
}

/**
* Generates string classKey based on the properties provided. It starts with the
* variant if defined, and then it appends all other properties in alphabetical order.
*
* @param {object} props - the properties for which the classKey should be created
*/
export default function propsToClassKey(props) {
const { variant, ...rest } = props;

let classKey = variant || '';

Object.keys(rest)
.sort()
.forEach((key) => {
if (key === 'color') {
classKey += isEmpty(classKey) ? props[key] : capitalize(props[key]);
} else {
classKey += `${isEmpty(classKey) ? key : capitalize(key)}${capitalize(props[key])}`;
}
});

return classKey;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { expect } from 'chai';
import propsToClassKey from './propsToClassKey';

describe('propsToClassKey', () => {
it('should return the variant value as string', () => {
expect(propsToClassKey({ variant: 'custom' })).to.equal('custom');
});

it('should combine the variant with other props', () => {
expect(propsToClassKey({ variant: 'custom', size: 'large' })).to.equal('customSizeLarge');
});

it('should append the props after the variant in alphabetical order', () => {
expect(propsToClassKey({ variant: 'custom', size: 'large', mode: 'static' })).to.equal(
'customModeStaticSizeLarge',
);
});

it('should not prefix the color prop', () => {
expect(propsToClassKey({ variant: 'custom', color: 'primary' })).to.equal('customPrimary');
});

it('should work without variant in props', () => {
expect(propsToClassKey({ color: 'primary', size: 'large', mode: 'static' })).to.equal(
'primaryModeStaticSizeLarge',
);
});

it('should not capitalize the first prop ', () => {
expect(propsToClassKey({ size: 'large', zIndex: 'toolbar' })).to.equal(
'sizeLargeZIndexToolbar',
);
});
});
Loading