[Button] Custom variant #21648
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| 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: [ | ||
| { | ||
| matcher: { variant: 'dashed' }, | ||
| styles: { | ||
| padding: '5px 15px', | ||
| border: `5px dashed ${defaultTheme.palette.primary.main}`, | ||
| }, | ||
| }, | ||
| { | ||
| matcher: { variant: 'dashed', color: 'secondary' }, | ||
| styles: { | ||
| border: `5px dashed ${defaultTheme.palette.secondary.main}`, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
|
|
||
| export default function GlobalThemeVariants() { | ||
| const classes = useStyles(); | ||
|
|
||
| return ( | ||
| <div className={classes.root}> | ||
| <ThemeProvider theme={inputTheme}> | ||
| <Button variant="dashed">Dashed</Button> | ||
| <Button variant="dashed" color="secondary"> | ||
| Dashed secondary | ||
| </Button> | ||
| </ThemeProvider> | ||
| </div> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| 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: { | ||
| '& > *': { | ||
|
There was a problem hiding this comment. Mentioning this for later, I have seen a growing trend toward the |
||
| margin: theme.spacing(1), | ||
| }, | ||
| }, | ||
| })); | ||
|
|
||
| const defaultTheme = createMuiTheme(); | ||
|
|
||
| const inputTheme = createMuiTheme({ | ||
| variants: { | ||
| MuiButton: [ | ||
| { | ||
| matcher: { variant: 'dashed' }, | ||
| styles: { | ||
| padding: '5px 15px', | ||
| border: `5px dashed ${defaultTheme.palette.primary.main}`, | ||
| }, | ||
| }, | ||
| { | ||
| matcher: { variant: 'dashed', color: 'secondary' }, | ||
| styles: { | ||
| border: `5px dashed ${defaultTheme.palette.secondary.main}`, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
|
|
||
| export default function GlobalThemeVariants() { | ||
| const classes = useStyles(); | ||
|
|
||
| return ( | ||
| <div className={classes.root}> | ||
| <ThemeProvider theme={inputTheme}> | ||
| <Button variant="dashed">Dashed</Button> | ||
| <Button variant="dashed" color="secondary"> | ||
| Dashed secondary | ||
| </Button> | ||
| </ThemeProvider> | ||
| </div> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -408,7 +408,10 @@ Button.propTypes = { | |
| /** | ||
| * The variant to use. | ||
| */ | ||
| variant: PropTypes /* @typescript-to-proptypes-ignore */.oneOfType([ | ||
| PropTypes.oneOf(['contained', 'outlined', 'text']), | ||
| PropTypes.string, | ||
|
There was a problem hiding this comment. We don't have to solve this right now but we should probably narrow this down before release by checking if any matcher matched. Right now vanilla users don't get runtime validation if the added the wrong There was a problem hiding this comment. Thinking about this, we can actually use the There was a problem hiding this comment. Would looking at
There is a new warning added by Marija that aim to mitigate with the problem (we get a warning but we don't have the full component stack trace) There was a problem hiding this comment. I was thinking as these validations are per prop, we can check for only that prop in the classes? But if I understand your point correctly, it can exists in combination with other props, not just itself, right? In that case I will need to update the warning I added, because currently it is checking only for the variant itself. There was a problem hiding this comment. My assumptions were:
There was a problem hiding this comment.
We can add this even now, with a simple hook like: const useThemeVariants = (props, name) => {
const theme = useTheme();
const variantsClasses = [];
if(theme && theme.variants && theme.variants[name]) {
const themeVariants = theme.variants[name];
themeVariants.forEach(themeVariant => {
let isMatch = true;
Object.keys(themeVariant.props).forEach(key => {
if(props[key] !== themeVariant.props[key]) {
isMatch = false;
}
});
if(isMatch) {
variantsClasses.push(classes[propsToClassKey(themeVariant.props)]);
}
})
}
return variantsClasses;
}Some of the things are recalcuated two times but I will refactor it if we want to go with something like this. This will allow us to even support new props 👍 I can clean this up and add it in the PR even now. For the warnings, we can extend it further, but I would like to see what would be the feedback, as I don't want to add a lot of overload for just warning, the one that is in place now it's pretty simple and I am not sure if we will need more complicated one.. There was a problem hiding this comment. Testing more this hook, I definitely think we should add it, as it will guarantee that whatever combination of props the clients will use, will be supported inside the component. @oliviertassinari @eps1lon thoughts? There was a problem hiding this comment. I think that it makes sense. As far as I can think about it, for JSS, we will need to both generate the class names from the theme, and to apply them from the props, I don't see any other way. For styled-components, we will be able to bundle the logic at the same place, under the styled call. There was a problem hiding this comment. I've added this in the |
||
| ]), | ||
| }; | ||
|
|
||
| export default withStyles(styles, { name: 'MuiButton' })(Button); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import { CSSProperties, CreateCSSProperties, PropsFunc } from '@material-ui/styles/withStyles'; | ||
| import { ComponentsPropsList } from './props'; | ||
|
|
||
| export type Variants = { | ||
| [Name in keyof ComponentsPropsList]?: Array<{ | ||
| matcher: Partial<ComponentsPropsList[Name]>; | ||
| styles: // JSS property bag | ||
| | CSSProperties | ||
| // JSS property bag where values are based on props | ||
| | CreateCSSProperties<Partial<ComponentsPropsList[Name]>> | ||
| // JSS property bag based on props | ||
| | PropsFunc< | ||
| Partial<ComponentsPropsList[Name]>, | ||
| CreateCSSProperties<Partial<ComponentsPropsList[Name]>> | ||
| >; | ||
| }>; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stringshouldn't be allowed by default. Will take a look later. Nothing that holds back this PR since it would also affect breakpoints.