[Button] Migrate styles to emotion by mnajdova · Pull Request #24107 · mui/material-ui

mnajdova · 2020-12-23T14:30:26Z

This PR migrates the Button's styles to emotion. In addition, it fixes #24045 - it's adding global class names for the default values of the size and color props.

…motion

mui-pr-bot · 2020-12-23T14:33:12Z

@material-ui/core: parsed: +0.74% , gzip: +0.56%
@material-ui/lab: parsed: +0.76% , gzip: +0.63%

Details of bundle changes

Generated by 🚫 dangerJS against 4205a69

mnajdova · 2020-12-23T14:34:19Z

packages/material-ui/src/Button/Button.test.js

+    let getByRole;
+
+    expect(() => {
+      const { getByRole: getByRoleLocal } = render(<Button startIcon={<span>icon</span>}>Hello World</Button>);


Emotion throws this error when ":first-child" is used. The error is shown when startIcon od endIcon is used.

Is it because emotion injects <style> elements in the page when SSR isn't configured?

emotion-js/emotion#1178

Exactly, but as we are following the advanced SSR configuration - https://emotion.sh/docs/ssr#advanced-approach we should be fine.

Managed to avoid the warning with 745e163. See comment - #24107 (comment)

mnajdova · 2020-12-23T15:09:34Z

packages/material-ui/src/Button/Button.test.js

+        <Button startIcon={<span>icon</span>}>Hello World</Button>,
+      );
+      getByRole = getByRoleLocal;
+    }).toErrorDev(


@eps1lon am I using the toErrorDev correctly here? If I have this expected error in both tests as it's done currently, the tests are failing with https://app.circleci.com/pipelines/github/mui-org/material-ui/30240/workflows/2f2f8de6-1ef0-48e1-9962-9baa4d91821e/jobs/206347

If I remove the toErrorDev in the second test, then it is failing with

19) <Button /> should render a button with endIcon: Expected test not to call console.error() If the warning is expected, test for it explicitly by using the toErrorDev() matcher. Test location: D:\workspace\material-ui\packages\material-ui\src\Button\Button.test.js console.error message: The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type". Stack: ...

Should I clear the errors in between somewhere or?

Is emotion deduplicating errors at the module level or something?

I wouldn't worry about it since we don't want to ship that warning anyway. SSR should not have any warnings or errors by default.

Fixed the warning with 745e163 I've replaced the '& > *:first-child' selctor with '& > *:nth-of-type(1)' to avoid this warning. The result should be identical.

The result should be identical.

More or less 😁 https://codepen.io/oliviertassinari/pen/oNYBeEd

mnajdova · 2020-12-23T18:42:39Z

Regarding the argos change - https://www.argos-ci.com/mui-org/material-ui/builds/6382 seems like we are fixing a previous issue with these changes

Jack-Works · 2020-12-31T04:16:20Z

I found it is a breaking change in our code. The problem is related to CSS priority. It seems like CSS is injected in the wrong order

Jack-Works · 2020-12-31T04:19:29Z

The problem is that makeStyles inserted after the emotion styles which is wrong. (My styles are overwritten by emotion-based MUI styles)

Jack-Works · 2020-12-31T04:34:44Z

How to make sure MUI styles (despite JSS or emotion) is always inserted before developer styles (despite JSS or emotion)? I can't find a workaround, is there an official guide for it? @oliviertassinari

Jack-Works · 2020-12-31T07:07:20Z

Well I write a dirty workaround

const workaround = document.createElement('div')
document.body.insertBefore(workaround, null)
const shadowMap = new WeakMap<HTMLStyleElement, HTMLStyleElement>()
document.head.insertBefore = new Proxy(document.head.insertBefore, {
    apply(f, target, [dom, before]) {
        if (target === document.head && dom instanceof HTMLStyleElement) {
            if (shadowMap.has(before)) before = shadowMap.get(before)
            const meta = dom.getAttribute('data-meta')
            if (meta && meta.startsWith('makeStyle')) {
                workaround.appendChild(dom)
                const shadow = document.createElement('style')
                shadowMap.set(dom, shadow)
                Reflect.apply(f, target, [shadow, before])
                Object.defineProperty(dom, 'parentNode', {
                    get() {
                        return document.head
                    },
                })
                return dom
            }
        }
        return Reflect.apply(f, target, [dom, before])
    },
})

mnajdova · 2020-12-31T08:25:44Z

@Jack-Works you need to use the <StylesProvider injectFirst> from @material-ui/core that will make sure that emotion styles are injected first. You can see this issue too #24186

Jack-Works · 2020-12-31T11:50:17Z

<StylesProvider injectFirst>

I used it but it doesn't work. The problem here is both JSS and emotion can have user styles and MUI styles.

mnajdova · 2020-12-31T11:55:07Z

I used it but it doesn't work. The problem here is both JSS and emotion can have user styles and MUI styles.

Yes, it cannot work if you use both style engines for overrides randomly. I would suggest if you plan to migrate to the new overrides technique, to do it step by step as we migrate the components. If you don't want to do it iterativly, I'd suggest keeping the JSS overrides and migrate in the end to emotion.

The problematic thing could be the theme overrides, as those are already using emotion. Is this the problem you are facing?

Jack-Works · 2020-12-31T12:04:02Z

I'm not quite clear. My workaround works for me currently. I'll continue to use it until makeStyles moved to emotion.

oliviertassinari · 2020-12-31T12:26:08Z

I'll continue to use it until makeStyles moved to emotion.

Mind that we are removing makeStyles progressively (v6), there are no plans to port this API to emotion/sc.

Jack-Works · 2020-12-31T13:08:56Z

Mind that we are removing makeStyles progressively (v6), there are no plans to port this API to emotion/sc.

Wait, all of our custom CSS are written in makeStyles, how can we migrate? that work is too heavy if it cannot be done with a codemod

oliviertassinari · 2020-12-31T13:15:29Z

@Jack-Works react-jss provides the same API, it will be the simplest path for developers that want a quick-win. But we won't drop the API before v6 so you have time.

Jack-Works · 2021-01-03T11:25:40Z

Ok I found another problem. MenuItem is based on ButtonBase. And now the CSS order in our app causes problem:

makeStyles > any emotion style > rest JSS styles. Now CSS of built-in MenuItem style is overwritten by ButtonBase style which is wrong. I want to know which order is correct. Should I use makeStyles > rest JSS styles > any emotion styles?

oliviertassinari · 2021-01-03T11:29:24Z

Should I use makeStyles > rest JSS styles > any emotion styles?

@Jack-Works It's what we recommend, so far. I have updated #24229 to mention it, in hindsight.

mnajdova added 21 commits December 22, 2020 14:08

init

b111e71

TouchRipple converted

7a6f9e0

touch ripple migration & test fixes

486fb48

fixed kebab case css props

98bc9cc

fixed child classes

ee61bfa

prettier & docs:api

735387f

adde touchRippleClasses d file and exported

3e68ac9

fixed typo

d13384c

StylesProvider fixes wip, TouchRipple string template

6163c53

prettier

3151d7f

Merge branch 'next' into feat/button-base-emotion

2367f1a

wip

4c98597

revert StylesProvider changes

ed8e2e5

replaced with ts-expect-error

162367f

more changes

1750f9a

Merge branch 'feat/button-base-emotion' into feat/button-migrate-to-e…

9e9e9ea

…motion

fixes

f5834e4

test, proptypes, docs:api

121afee

test fixes

662f336

test

dfe0e1c

Merge branch 'next' into feat/button-migrate-to-emotion

1faa4b4

mnajdova commented Dec 23, 2020

View reviewed changes

mnajdova requested review from eps1lon and oliviertassinari December 23, 2020 14:35

prettier

b722c5d

github-actions bot mentioned this pull request Dec 23, 2020

Update snapshots for pr/24107 eps1lon/mui-scripts-incubator#1578

Closed

mnajdova commented Dec 23, 2020

View reviewed changes

replaced first-child selector

745e163

oliviertassinari approved these changes Dec 23, 2020

View reviewed changes

fixed argos diffs, converted customized button example to use styled()

4205a69

eps1lon approved these changes Dec 23, 2020

View reviewed changes

mnajdova merged commit ee64239 into mui:next Dec 24, 2020

oliviertassinari mentioned this pull request Dec 24, 2020

[Avatar] Migrate to emotion #24114

Merged

oliviertassinari mentioned this pull request Jan 3, 2021

[Button] Fix disableElevation regression #24251

Merged

oliviertassinari mentioned this pull request Feb 13, 2021

[Tabs] pseudo class ":first-child" is potentially unsafe #24894

Closed

zannager added the scope: button Changes related to the button. label Jan 27, 2023

Uh oh!

Conversation

mnajdova commented Dec 23, 2020

Uh oh!

mui-pr-bot commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnajdova Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnajdova commented Dec 23, 2020

Uh oh!

Jack-Works commented Dec 31, 2020

Uh oh!

Jack-Works commented Dec 31, 2020

Uh oh!

Jack-Works commented Dec 31, 2020

Uh oh!

Jack-Works commented Dec 31, 2020

Uh oh!

mnajdova commented Dec 31, 2020

Uh oh!

Jack-Works commented Dec 31, 2020

Uh oh!

mnajdova commented Dec 31, 2020

Uh oh!

Jack-Works commented Dec 31, 2020

Uh oh!

oliviertassinari commented Dec 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jack-Works commented Dec 31, 2020

Uh oh!

oliviertassinari commented Dec 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jack-Works commented Jan 3, 2021

Uh oh!

oliviertassinari commented Jan 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mui-pr-bot commented Dec 23, 2020 •

edited

Loading

mnajdova Dec 23, 2020 •

edited

Loading

oliviertassinari commented Dec 31, 2020 •

edited

Loading

oliviertassinari commented Dec 31, 2020 •

edited

Loading