[Checkbox] Fix custom icon fontSize prop support #21362
Merged
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
Details of bundle changes.Comparing: edb2d63...31be959 Details of page changes
|
kn1ves
changed the title
oliviertassinari
added
component: checkbox
oliviertassinari
changed the title
|
@oliviertassinari The following example is now working:
Please review the changes in the test case :)
|
oliviertassinari
changed the title
oliviertassinari
added
PR: ready to ship
and removed
component: radio
oliviertassinari
approved these changes
Jun 9, 2020
eps1lon
added
PR: needs test
eps1lon
requested changes
Jun 9, 2020
@kn1ves Thanks for testing that path out. It was my bad, we don't face this problem with the radio component. Sorry for leading you in a dead-end from the issue discussion. |
There were no changes required in the Radio component, it is working as intended. The only changes made were in the Checkbox component. |
|
@kn1ves I think that what Sebastian meant was that the visual regression test might be "light", we could easily add a functional test, like testing the right class name is applied. The best proof of that argument is that we broke the visual test in the past without noticing (and are now fixing it) |
|
@oliviertassinari Ohh okay, I'll take a look into it. Thanks :) |
|
@kn1ves Heads up, I'm adding a quick test so we can include it in one of the last releases of v4.x |
oliviertassinari
added
PR: ready to ship
and removed
PR: needs test
oliviertassinari
force-pushed
the
icon-size-override
branch
from
36643ef
to
31be959
Compare
Alright, do you still need me to work on creating a test? I wasn't familiar with chai otherwise I'd have one created by now, sorry for the delay. |
|
@kn1ves No problem, I think that we are good now :) |
|
@kn1ves It's a great first pull request on Material-UI 👌🏻. Thank you for working on it! |
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 tried a couple of different things and this solution works great. I almost feel guilty because I couldn't really change much from the original solution provided by @oliviertassinari except the fact that
size: PropTypes.oneOf(['medium', 'small']),doesn't require any changes since this refers to the size prop of the Checkbox component and the issue was caused by the fontSize prop of the icon being used.Also, I tested the Radio component for the same issue and it seemed to be working fine, unless I'm missing something obvious?
Closes #21289