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.
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 11 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
There are no files selected for viewing
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.
It sounds like a method that already exists. I assume you wanted to avoid circular dependencies. We could solve the circular dependency be moving capitalize to the
@material-ui/utils
repository.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.
I moved this to
@material-ui/utils
and re-exported this undermaterial-ui/core/utils
, in order to avoid to many changes of the imports with this PR. We can remove that later if we need to.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.
Seems like there are still circular dependencies, I believe it's because I am importing there:
import MuiError from '../macros/MuiError.macro';
🤔 How can I fix this?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.
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
variant
.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.
Thinking about this, we can actually use the
chainPropTypes
and check if theprops.variant
can be found in theprops.classes
. That way I think we can even get rid of the warnings we have currently in place. What do you think about this?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.
Would looking at
props.classes
still work if we support arbitrary props combination (variant + size + orientation) or if the style engine is styled-components?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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My assumptions were:
chainPropTypes
to have a warning with the full component trace. We would need a new primitive from React. So if we can't get it with sc, maybe we shouldn't try with JSS in the first place.<Typography variant="headline1" size="small" color="brand" />
or any dynamic matching props, we would need to construct the class names dynamically, in this case, maybe.MuiTypogaphy-headline1SizeSmallColorBrand
.MuiTypography
+variant="headline1" />
when the headline1 variant doesn't exist, this help a lotThere 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.
We can add this even now, with a simple hook like:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this in the
withStyles
utility, it matches which classes should we added and appends them in theclasses.root
string. Please take a look and let me know if this makes sense.