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 64 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>
);
}
38 changes: 38 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,41 @@ 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.
eps1lon marked this conversation as resolved.
Show resolved Hide resolved

```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 '../propsToClassKeys';
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.`,
mnajdova marked this conversation as resolved.
Show resolved Hide resolved
`Please use the \`theme.variants.${name}\` to define a new entry for this variant.`,
mnajdova marked this conversation as resolved.
Show resolved Hide resolved
].join('\n'),
);
}
}

return classes;
};
Expand Down
2 changes: 2 additions & 0 deletions packages/material-ui-styles/src/propsToClassKeys/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { default } from './propsToClassKeys';
export * from './propsToClassKeys';
1 change: 1 addition & 0 deletions packages/material-ui-styles/src/propsToClassKeys/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './propsToClassKeys';
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default function propsToClassKeys(props: object): string;
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
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;
}

export default function propsToClassKey(props) {
mnajdova marked this conversation as resolved.
Show resolved Hide resolved
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;
}
22 changes: 21 additions & 1 deletion packages/material-ui-styles/src/withStyles/withStyles.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { chainPropTypes, getDisplayName } from '@material-ui/utils';
import makeStyles from '../makeStyles';
import getThemeProps from '../getThemeProps';
import useTheme from '../useTheme';
import propsToClassKey from '../propsToClassKeys';

// Link a style sheet with a component.
// It does not modify the component passed to it;
Expand Down Expand Up @@ -48,7 +49,7 @@ const withStyles = (stylesOrCreator, options = {}) => (Component) => {
// The wrapper receives only user supplied props, which could be a subset of
// the actual props Component might receive due to merging with defaultProps.
// So copying it here would give us the same result in the wrapper as well.
const classes = useStyles({ ...Component.defaultProps, ...props });
let classes = useStyles({ ...Component.defaultProps, ...props });

let theme;
let more = other;
Expand All @@ -67,6 +68,25 @@ const withStyles = (stylesOrCreator, options = {}) => (Component) => {
if (withTheme && !more.theme) {
more.theme = theme;
}

if (theme && theme.variants && theme.variants[name]) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@oliviertassinari here is the logic for the dynamic classes.

Copy link
Member

Choose a reason for hiding this comment

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

Considering this failing test case:

    it.only('should map the variant classkey to the component', function test() {
      if (/jsdom/.test(window.navigator.userAgent)) {
        // see https://github.com/jsdom/jsdom/issues/2953
        this.skip();
      }

      const theme = createMuiTheme({
        variants: {
          MuiButton: [
            {
              props: { variant: 'test', color: 'primary', size: 'large' },
              styles: { backgroundColor: 'rgb(255, 0, 0)' },
            },
          ],
        },
      });

      render(<WrappedComponent theme={theme} variant="test" size="large" />);

      const style = window.getComputedStyle(screen.getByTestId('component'));
      expect(style.getPropertyValue('background-color')).to.equal('rgb(255, 0, 0)');
    });

Should we move the logic outside of withStyles, after the resolution of the default props?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me test this out a bit... The defaults for this are set inside the render method of the component, so we can potentially move it there, but that would mean that it needs to be added as in each component separately (which won't be a big deal as it can be implemented as a hook). Let me test this tomorrow and ensure that it works

Copy link
Member Author

Choose a reason for hiding this comment

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

Related question, why are we using sometimes the static defailtProps, and other times defaults in the render method (the second should be better as defaultProps will be removed`), but it is confusing - I thought initially that it is default to spread the defaultProps there, as it was implemented like that previosly, but that is obviously not enough.

let variantsClasses = '';
const themeVariants = theme.variants[name];

themeVariants.forEach((themeVariant) => {
let isMatch = true;
Object.keys(themeVariant.props).forEach((key) => {
if (more[key] !== themeVariant.props[key]) {
isMatch = false;
}
});
if (isMatch) {
variantsClasses = `${variantsClasses} ${classes[propsToClassKey(themeVariant.props)]}`;
}
});

classes = { ...classes, root: `${classes.root} ${variantsClasses}` };
}
}

return <Component ref={innerRef || ref} classes={classes} {...more} />;
Expand Down
Loading