[Icon] Migrate to emotion

[queengooborg]

This PR migrates the Icon component to emotion, with code heavily based upon #24506. Part of #24405.

• I have followed (at least) the PR section of the contributing guide.

[mui-pr-bot]

@material-ui/core: parsed: +0.14% , gzip: +0.17%

Details of bundle changes

Generated by 🚫 dangerJS against 2957c76

[mnajdova]

Only a couple of comments generally looks good :) Great progress!

[mnajdova]

Something is off with the fontSize property https://codesandbox.io/s/icons-material-demo-forked-c3t38?file=/demo.js We also have this regression:

This PR - https://deploy-preview-24516--material-ui.netlify.app/components/icons/#font-material-icons
Next - https://next.material-ui.com/components/icons/#font-material-icons

[queengooborg]

Ahh, I see what's going on -- it appears that the font size is getting overwritten by the CSS for .material-icons (the baseClassName). Thus, the fontSize property is essentially just ignored.

I'm guessing the preferred option is to pseudo-class the fontSize property to increase the styling rule's weight, making a commit now!

[mnajdova]

Ahh, I see what's going on -- it appears that the font size is getting overwritten by the CSS for .material-icons (the baseClassName). Thus, the fontSize property is essentially just ignored.

I'm guessing the preferred option is to pseudo-class the fontSize property to increase the styling rule's weight, making a commit now!

Thanks for taking a look on it! Yes, it appears that this resource - https://fonts.googleapis.com/icon?family=Material+Icons|Material+Icons+Two+Tone is added after the styles coming from emotion, so it wins over our styles. We do not want to increase the specificity for the fontSize, please revert those changes, as the overrides would become hard for them. I'd say we need to change the location of where the tag for the icons is added (it should be before our styles) cc @oliviertassinari

Let's revert it to the initial implementation (without using class selectors) and add //TODO for fixing the order of the injected styles.

[queengooborg]

Sure thing, revert complete!

At first, I was thinking that it was the added fontSizeMedium class (which was copied from SVGIcon), but that class doesn't seem to be causing any harm (the font size is the same as what's defined on .material-icons). Would we want to keep that for consistency with SVGIcon, or keep it out since it wasn't originally around?

[mnajdova]

Would we want to keep that for consistency with SVGIcon, or keep it out since it wasn't originally around?

That, plus we want to have the utility classes for the default values as well in the component so that developers can target those and change them if they want to.

[queengooborg]

Looks like the last test failed due to an issue connecting to Argos CI from Circle CI. Can a peer restart the job?

[oliviertassinari]

@mnajdova Interesting issue with the CSS, it looks like an issue with the configuration of the documentation. I wish emotion allowed to configure the injection point as JSS does. It will be resolve once we have migrated everything, then we can remove the prepend: true flag.

[mnajdova]

🚀

[mnajdova]

@mnajdova Interesting issue with the CSS, it looks like an issue with the configuration of the documentation. I wish emotion allowed to configure the injection point as JSS does. It will be resolve once we have migrated everything, then we can remove the prepend: true flag.

@oliviertassinari emotion has the container element https://github.com/emotion-js/emotion/blob/master/packages/cache/README.md#container which means we should be able to specify for the styles to be injected after those, but let's see if we will still need once we don't have the prepend option.

[oliviertassinari]

@mnajdova As far as I understand the container option, it doesn't help. It's meant for solving cross-window use cases (iframe, popup, shadow-dom). We can only set it to be document.head. Maybe we should report this to the team? With styled-components, it can be solved by creating an empty <style data-styled="active"> and putting it in the right position.

[mnajdova]

@mnajdova As far as I understand the container option, it doesn't help. It's meant for solving cross-window use cases (iframe, popup, shadow-dom). We can only set it to be document.head. Maybe we should report this to the team? With styled-components, it can be solved by creating an empty <style data-styled="active"> and putting it in the right position.

Ah you are correct, I was mixing it with styled-components. Mentioning @Andarist for visibility. We were wondering whether there is an option for setting a specific node where we want the styles coming from emotion to be injected.

In our case, it would be after where the Material Design font icons are injected and before where JSS is injected:

<head>
  <style>Material-Design-Icons</style>
  <style>Emotion</style>
  <style>JSS</style>

[Andarist]

Hmm, even if it looks odd - you could create a div in your head and pass that div as the container to us.

Could you point me to JSS docs that describe the insertion point option? How exactly would you configure it in your specific case? I'm interested in seeing what exact elements would have to exist prior to instantiating your Emotion instance etc.

[oliviertassinari]

How exactly would you configure it in your specific case

@Andarist We do the following with JSS:

in the head:
https://github.com/mui-org/material-ui/blob/168b6d9b97aab99550a7d9a920cb53758bb4b980/docs/pages/_document.js#L170-L174
in the configuration:

https://github.com/mui-org/material-ui/blob/11b10cd570f75147bf19dec99f2e26387b7da767/docs/pages/_app.js#L28-L32

[Andarist]

I wasn't super keen on introducing an additional option for this but when I now think amount migrating to Emotion, having different styles etc it makes sense to support something like this (and to deprecate prepend in the process).

Would your team be able to help out implementing this? This would only require adding a new option (probably normalizing it into a common option) and adjusting the _insertTag:
https://github.com/emotion-js/emotion/blob/400c6f72905a4676f852b09405421a7ca8d7a3a2/packages/sheet/src/index.js#L88

[oliviertassinari]

@Andarist It seems that once we have migrated all the components to emotion, we won't need, for the docs, to have this option. Still, developers might need it, I think that we could consider contributing this back, thanks for the proposal!
Now, I'm not sure it's more important than the Global issue we surfaced a couple of weeks ago (but it seems simpler to fix).

[Andarist]

Oh ye - definitely the other one might is more important overall but also a little bit more involved. I'm open to discussing how that Global issue should be solved, I would probably handle it myself but lately, I'm just drowning in GitHub notifications and I struggle to keep up with them so I'm not sure when I will have time to sit down to it properly.