[Chip] Migrate to emotion #24649
[Chip] Migrate to emotion #24649
Conversation
|
@material-ui/core: parsed: +0.48% , gzip: +0.27% |
| [classes[`iconColor${capitalize(color)}`]]: color !== 'default', | ||
| }), | ||
| }); | ||
| icon = ( |
There was a problem hiding this comment.
Watch out, here you are creating a wrapper around the original component. We have two options of how to solve this:
- Add the styles in the root using the class selector for this slot - we would be increasing the specificity with this :\
- Another option would be to use the
asprop to render theiconProp, something like:
<ChipIcon
{...iconProp.props}
as={iconProp.type}
styleProps={styleProps}
className={clsx(classes.icon, iconProp.props.className)}
/>
Here is an example with working example - https://codesandbox.io/s/sad-wood-m4vfh?file=/src/App.js
I would vote for option 2, it should be the one without introducing any breaking changes. cc @oliviertassinari let me know if you agree.
There was a problem hiding this comment.
This case with the icon looks identical to the case with the avatar. I think that we should increase the specificity anytime we use cloneElement. I can search the issue that we had it the past with the avatar that lead to increasing the specificity.
There was a problem hiding this comment.
#18156 should give more context.
There was a problem hiding this comment.
We can always try option 2 and see how it goes
There was a problem hiding this comment.
I am always more scared to introduce bigger specificity, I'd say let's try not to if we see the problem we can do it later.. I am pretty sure we are soon going to receive complain if something is not working.. :)
There was a problem hiding this comment.
If that's the case then let's go with the bigger specificity
There was a problem hiding this comment.
@natac13 sorry for the confusion let's go with increasing the specificity, the root would contain the styles for the avatar, icon, and delteIcon. The label is anyway a different component.
There was a problem hiding this comment.
No worries. Ill make the changes. I hope 🤞🏻
There was a problem hiding this comment.
Basically, if we want to support custom external icons, we need to consider the following:
- With emotion, we get the specificity order with prop cascading: option 2 is OK
- With JSS, and sc with are getting the specificity with import order, CSS rules specificity is order dependent on chips #16374 (comment) shares an issue we had in the past. It was related to dead-code elimination. In order to get it to work, developers have to import the icon and the chip in the right order, it's error-prone and not obvious. @dtassone experienced this problem first hand with the data grid recently. option 2 is borderline
- With CSS we are getting injection order, developers will configure it to have more specificity, they won't have any luck. option 2 is KO
There was a problem hiding this comment.
@oliviertassinari would be nice if you can also do a final review - @natac13 already implemented the changes with increasing the specificity, looks good to me
oliviertassinari
added
the
component: chip
|
I still cannot figure out the 4 tests that are failing. |
Taking a look Edited: it was a matter of typo - fixed with 11bff3d |
Is this just based off the testing, or do you manually check it visually? |
I am mostly depending on argos for visual tests, unless the component has some state, then I try to test out the other states too. This one looks good |
It has allowed me to find one mistake with `outlined${capitalize(variant)}`
#24405
Will need a bit of help with this one.