-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Button] Custom variant #21648
Changes from 78 commits
97aac4e
dd57f8e
5c3d592
8ba964e
6e0ab4d
5c8364c
653c1e7
1755be6
8c0ae69
036a172
1b94f50
9370c1c
df60098
f430770
2375c98
7db6a98
c032672
22d5f4a
3fcb966
212db32
f92dbaa
ce2369f
0e67339
ccaf13f
070ffe0
a86137e
628e8fd
132c930
5036d2d
5e79126
fa883ea
0ab15a6
fed904b
9fcc431
313f8bf
60787ab
f04ccb4
1b81481
7f12e68
e1c2d6d
4f34120
4f0a648
beab317
0c00ea9
70b713d
bb687a4
4efaf62
18de558
4cb0b03
e780712
eb02491
bb5baa1
3e1ea72
90609ff
c4df879
7d4fa1d
7ae76f7
c0a9d55
f888036
b26d20d
75cd3b6
5365b71
2916cd6
e185968
2b32fe8
efe2e7d
4ce2381
048cd39
57e4700
4196182
515933e
1d240c0
b9ac3ca
af088c9
4e8de25
20477c6
0aa43ad
c71e4ba
2bb3a29
52d11c8
55f0507
44e2f1d
a55dfe4
6b1742a
bcae6ef
704e2f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
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 theme = createMuiTheme({ | ||
variants: { | ||
MuiButton: [ | ||
{ | ||
props: { variant: 'dashed' }, | ||
styles: { | ||
textTransform: 'none', | ||
border: `2px dashed ${defaultTheme.palette.primary.main}`, | ||
color: defaultTheme.palette.primary.main, | ||
}, | ||
}, | ||
{ | ||
props: { variant: 'dashed', color: 'secondary' }, | ||
styles: { | ||
border: `2px dashed ${defaultTheme.palette.secondary.main}`, | ||
color: defaultTheme.palette.secondary.main, | ||
}, | ||
}, | ||
{ | ||
props: { variant: 'dashed', size: 'large' }, | ||
styles: { | ||
borderWidth: 4, | ||
}, | ||
}, | ||
{ | ||
props: { variant: 'dashed', color: 'secondary', size: 'large' }, | ||
styles: { | ||
fontSize: 18, | ||
}, | ||
}, | ||
], | ||
}, | ||
}); | ||
|
||
export default function GlobalThemeVariants() { | ||
const classes = useStyles(); | ||
|
||
return ( | ||
<div className={classes.root}> | ||
<ThemeProvider theme={theme}> | ||
<Button variant="dashed">Dashed</Button> | ||
<Button variant="dashed" color="secondary"> | ||
Secondary | ||
</Button> | ||
<Button variant="dashed" size="large"> | ||
Large | ||
</Button> | ||
<Button variant="dashed" color="secondary" size="large"> | ||
Secondary large | ||
</Button> | ||
</ThemeProvider> | ||
</div> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mentioning this for later, I have seen a growing trend toward the |
||
margin: theme.spacing(1), | ||
}, | ||
}, | ||
})); | ||
|
||
const defaultTheme = createMuiTheme(); | ||
|
||
const theme = createMuiTheme({ | ||
variants: { | ||
MuiButton: [ | ||
{ | ||
props: { variant: 'dashed' }, | ||
styles: { | ||
textTransform: 'none', | ||
border: `2px dashed ${defaultTheme.palette.primary.main}`, | ||
color: defaultTheme.palette.primary.main, | ||
}, | ||
}, | ||
{ | ||
props: { variant: 'dashed', color: 'secondary' }, | ||
styles: { | ||
border: `2px dashed ${defaultTheme.palette.secondary.main}`, | ||
color: defaultTheme.palette.secondary.main, | ||
}, | ||
}, | ||
{ | ||
props: { variant: 'dashed', size: 'large' }, | ||
styles: { | ||
borderWidth: 4, | ||
}, | ||
}, | ||
{ | ||
props: { variant: 'dashed', color: 'secondary', size: 'large' }, | ||
styles: { | ||
fontSize: 18, | ||
}, | ||
}, | ||
], | ||
}, | ||
}); | ||
|
||
export default function GlobalThemeVariants() { | ||
const classes = useStyles(); | ||
|
||
return ( | ||
<div className={classes.root}> | ||
<ThemeProvider theme={theme}> | ||
<Button variant="dashed">Dashed</Button> | ||
<Button variant="dashed" color="secondary"> | ||
Secondary | ||
</Button> | ||
<Button variant="dashed" size="large"> | ||
Large | ||
</Button> | ||
<Button variant="dashed" color="secondary" size="large"> | ||
Secondary large | ||
</Button> | ||
</ThemeProvider> | ||
</div> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So even though both components have a At least we should have a descriptive warning if one wants to extend variants in components where this isn't supported. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Tested locally with this example
Update: aftre we introduced the hook for the custom variants, the hook will also need to be added in the components There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export { default } from './propsToClassKey'; | ||
export * from './propsToClassKey'; |
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; |
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', | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export { default } from './useThemeVariants'; | ||
export * from './useThemeVariants'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { default } from './useThemeVariants'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default function useThemeVariants(props: object, name: string): string; |
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.
string
shouldn't be allowed by default. Will take a look later. Nothing that holds back this PR since it would also affect breakpoints.