[styles] Warn when using a missing style rule #16501
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oliviertassinari
commented
Jul 6, 2019
| @@ -178,7 +178,7 @@ function MarkdownDocs(props) { | |||
| </Portal> | |||
| )} | |||
|
|
|||
| <AppContent disableToc={disableToc} className={classes.root}> | |||
| <AppContent disableToc={disableToc}> | |||
There was a problem hiding this comment.
A dead class name found with the new warning.
There was a problem hiding this comment.
If you're concerned with using missing classes then using types is the better approach.
eps1lon
reviewed
Jul 6, 2019
| @@ -221,7 +222,7 @@ Typography.propTypes = { | |||
| /** | |||
| * Applies the theme typography styles. | |||
| */ | |||
| variant: PropTypes.oneOf([ | |||
| variant: oneOfWithString([ | |||
There was a problem hiding this comment.
How does react-docgen handle PropTypes.oneOfType([PropTypes.string, PropTypes.oneOf([...]])
|
Without For custom variants we have to defer checking to the style handler anyway. Not sure why we need a Proxy for that. |
|
Yeah, I'm not very happy with this approach. Another one I can think of. We could have a style method that internalize the warning logic: diff --git a/packages/material-ui/src/Typography/Typography.js b/packages/material-ui/src/Typography/Typography.js
index 132001221..b75fe1486 100644
--- a/packages/material-ui/src/Typography/Typography.js
+++ b/packages/material-ui/src/Typography/Typography.js
@@ -146,9 +146,13 @@ const Typography = React.forwardRef(function Typography(props, ref) {
<Component
className={clsx(
classes.root,
+ style({
+ classes,
+ key: `color${capitalize(color)}`,
+ enable: color !== 'initial',
+ }),
{
[classes[variant]]: variant !== 'inherit',
- [classes[`color${capitalize(color)}`]]: color !== 'initial',
[classes.noWrap]: noWrap,
[classes.gutterBottom]: gutterBottom,
[classes.paragraph]: paragraph, |
|
I will resume the effort later on. |
zannager
added
the
package: styles
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm exploring one possible approach to better handle #13875, #15454, #15573.
The idea here is to soften the
PropTypes.oneOfprop type toPropTypes.stringwhile keeping the warning for dynamic values with aclassesproxy that detects accesses to none supported values.However, I have found two drawbacks with this approach:
classesreturn a rich object, it's less obvious that it's a simple object behind the proxy: