[Modal] Migrate to emotion + unstyled #24857

povilass · 2021-02-10T17:06:54Z

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

Closes #20957
Closes #16442

mui-pr-bot · 2021-02-10T17:09:16Z

@material-ui/core: parsed: +0.37% , gzip: +0.23%
@material-ui/lab: parsed: +0.38% , gzip: +0.41%
@material-ui/unstyled: parsed: +26.65% , gzip: +23.41%

Details of bundle changes

Generated by 🚫 dangerJS against 13ffe70

povilass · 2021-02-10T18:14:48Z

Any ideas why karma fails?

Firefox 78.0 (Windows 10) ERROR
  ReferenceError: before is not defined
  at webpack:///test/utils/setupKarma.js:17:1 <- test/karma.tests.js:351818:1

Seen other pull request but their just fail because of timeout https://app.circleci.com/pipelines/github/mui-org/material-ui/37775/workflows/a3c068e5-582a-4e66-9564-d7f0eccd2d54/jobs/223216

Firefox 78.0 (Windows 10) ERROR
  Disconnectedreconnect failed before timeout of 120000ms (transport close)
Firefox 78.0 (Windows 10): Executed 2945 of 3434 (skipped 125) DISCONNECTED (6 mins 45.288 secs / 3 mins 46.492 secs)

oliviertassinari · 2021-02-10T18:34:14Z

@povilass It has been failing like this for a week. I can even reproduce it locally. I was planning to solve/spend time on this issue. There is a high variance in the builds in BrowserStack, for some reason, sometimes it runs in 40s, sometimes in 3min+. cc @eps1lon CircleCI isn't the only one with unstable builds durations 🙃

povilass · 2021-02-10T18:45:07Z

@oliviertassinari well that's disappointing, but at least I know code is not failing.

oliviertassinari

The modal is tricky. We can't simply migrate it to emotion as it would break its current value proposition. It's meant to be very lightweight without any style library dependencies.

I see two possible options:

We don't do anything about it, we keep the current inline style tradeoff
We add an unstyled version of it

povilass · 2021-02-11T06:02:23Z

@oliviertassinari so you can decide which are you preferring and we could close this pull request and I would open a new request if you want to migrate it to UnstyledModal. But keep in mind Modal uses theme to get zIndex.

oliviertassinari · 2021-02-11T13:03:24Z

@povilass I think that option 2 would be better longer-term. It would normalize the pattern. Right now, it's somewhat a hybrid. For instance, we have a SimpleBackdrop that uses inline style and a proper Backdrop that we inject when we need to use the Modal component in the other component, e.g. Dialog.

povilass · 2021-02-11T13:15:56Z

@oliviertassinari Ok got, I would suggest updating the v5 list with NO_NEED_FOR_CONVERSION and move this change to the next iteration to the Unstyled package.

oliviertassinari · 2021-02-11T13:20:42Z

@povilass Well, I still think that it needs to be migrated to emotion, but it needs the unstyled version too, otherwise, it's a regression for many developers using it as an alternative to https://github.com/reactjs/react-modal.

povilass · 2021-02-11T14:34:28Z

@oliviertassinari Ok gonna try to add Unstyled version too.

oliviertassinari · 2021-02-11T14:44:48Z

@povilass Sounds great, the Slider is a great one as a model.

povilass · 2021-02-11T18:50:07Z

@oliviertassinari I have 1 question about moving Modal to Unstyled package, it depends on Portal and TrapFocus I have to move those to unstyled package also otherwise I can not migrate Modal is it ok or not?

Update:
Also, it depends on BackdropComponents props.

I think we should first move TrapFocus, Portal and Backdrop(has Fade props too) to unstyled version.

oliviertassinari · 2021-02-11T19:06:43Z

Portal and TrapFocus are already unstyled. We could indeed move them to the other package (@material-ui/unstyled) into a different pull request.

povilass · 2021-02-11T19:15:25Z

Backdrop migrated to emotion, probably can be moved to unstyled also and backdrop depends on Fade which already is unstyled just a transition.

Fade, Portal and TrapFocus can be moved with different pull requests now.
Next will be Backdrop to unstyled with other pull request and eventually, we can move Modal with this one.

Or should we merge Modal to emotion now close this issue and moved everything with other pull requests to unstyled package one by one? Do not want to mix emotion and unstyled issues in this one pull.

If you will let me I can create pull requests for each component and move it slowly.

oliviertassinari · 2021-02-11T23:01:52Z

Backdrop migrated to emotion, probably can be moved to unstyled also and backdrop depends on Fade which already is unstyled just a transition.

@povilass Yes, I agree. For the Modal to be unstyled, it needs its leaves to be too.

Fade, Portal and TrapFocus can be moved with different pull requests now.

Fade is handled, we don't need to change it. It's using inline-style.

Or should we merge Modal to emotion now close this issue and moved everything with other pull requests to unstyled package one by one? Do not want to mix emotion and unstyled issues in this one pull.
If you will let me I can create pull requests for each component and move it slowly.

I think that we should move one component at a time, starting from the most nested blockers. I imagine the following possible iterations:

Move TrapFocus from core to unstyled
Move Portal from core to unstyled
Move Backdrop to emotion + unstyled
Move Modal to emotion + unstyled. Remove SimpleBackdrop. Update the demos to document both styled/unstyled components.

cc @eps1lon

povilass · 2021-02-12T06:02:15Z

@oliviertassinari I mean Backdrop depends on Fade props check and Fade depends on Transition props probably also can be moved to unstyled? Or BackdropUnstyled component can be without transition

UPDATE:

I think unstyled components should be without transition itself but you can pass components in core like Slider does with additional properties.

oliviertassinari · 2021-02-12T12:08:42Z

@povilass Yes, you got the idea. Transitions don't make sense for unstyled components. The styled Backdrop should inject it's transition.

povilass · 2021-02-19T15:53:41Z

Gonna move it to unstyled and remove a simple backdrop so don't need a review for now

povilass · 2021-02-20T17:18:15Z

TODO: Updating docs (for myself)

@oliviertassinari I have a problem with how to resolve exit property it's duplicate in both components. It depends on the transition component passed as a child. Should I move exit handling to Modal and pass it to ModalUnstyled as prop (both need to apply current styles and classes)?

povilass · 2021-02-20T20:56:05Z

There is a problem with typings where OverridableComponent returns JSX.Element.

declare const Backdrop: OverridableComponent<BackdropTypeMap>;
Backdrop -> JSX.Element

But BackdropComponent requires ComponentType or ElementType

In Modal.d.ts
    /**
     * A backdrop component. This prop enables custom backdrop rendering.
     * @default Backdrop
     */
    BackdropComponent?: React.ComponentType<BackdropProps>;

And can't resolve this issue because of this difference in SpringModal.tsx

UPDATE:
Adding changes to Modal -> BackdropComponent: React.ElementType work but if I add ModalUnstyled -> BackdropComponent: React.ElementType don't get it, is it typescript problem with overriding same property?

Index: packages/material-ui-unstyled/src/ModalUnstyled/ModalUnstyled.d.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- packages/material-ui-unstyled/src/ModalUnstyled/ModalUnstyled.d.ts	(revision f9712c1fdc04bb538703de2c56086d38572e1ed3)
+++ packages/material-ui-unstyled/src/ModalUnstyled/ModalUnstyled.d.ts	(date 1613855414514)
@@ -9,7 +9,7 @@
     /**
      * A backdrop component. This prop enables custom backdrop rendering.
      */
-    BackdropComponent?: React.ComponentType<BackdropUnstyledProps>;
+    BackdropComponent?: React.ElementType;
     /**
      * Props applied to the [`BackdropUnstyled`](/api/backdrop-unstyled/) element.
      */
@@ -136,7 +136,8 @@
  * Utility to create component types that inherit props from ModalUnstyled.
  */
 export interface ExtendModalUnstyledTypeMap<M extends OverridableTypeMap> {
-  props: M['props'] & ModalUnstyledTypeMap['props'];
+  // props: M['props'] & ModalUnstyledTypeMap['props'];
+  props: ModalUnstyledTypeMap['props'] & M['props'];
   defaultComponent: M['defaultComponent'];
 }
 
Index: packages/material-ui/src/Modal/Modal.d.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- packages/material-ui/src/Modal/Modal.d.ts	(revision f9712c1fdc04bb538703de2c56086d38572e1ed3)
+++ packages/material-ui/src/Modal/Modal.d.ts	(date 1613855212495)
@@ -14,7 +14,7 @@
      * A backdrop component. This prop enables custom backdrop rendering.
      * @default Backdrop
      */
-    BackdropComponent?: React.ComponentType<BackdropProps>;
+    BackdropComponent?: React.ElementType<BackdropProps>;
     /**
      * Props applied to the [`Backdrop`](/api/backdrop/) element.
      */

oliviertassinari · 2021-02-21T16:18:31Z

@povilass The fix looks good https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b3fb0eb579537bb20320e767a49ad2f3f661d43e/types/react/index.d.ts#L77

oliviertassinari · 2021-02-21T22:31:52Z

Rebased on HEAD.

povilass · 2021-02-22T06:43:59Z

Ok great, a lot of work done by this task did not even realize it will close other issues xd

oliviertassinari · 2021-02-22T11:17:17Z

@povilass Definitely fixing old issues :). What do you think about handling the documentation of unstyled before we merge?

povilass · 2021-02-22T11:43:23Z

I think both are needed. The question is why?
A styled component can introduce new properties or omit properties from ModalUnstyled properties
If you omit properties from unstyled components we will miss some documentation. If we leave only unstyled documentation we will miss documentation from added properties there (for example sx property) :(

Same problem with newly added classes which does not exist in the unstyled components.

oliviertassinari · 2021-02-22T14:24:03Z

@povilass I don't follow your point. For the documentation, I was referring to having a ## Unstyled modal demo after https://next.material-ui.com/components/modal/#simple-modal so the developers can see two options exist and how to apply the basic styles to get it working with a decent layout.

povilass · 2021-02-22T14:30:37Z

@oliviertassinari Oh ok, though you are talking about API documentation of Modal and ModalUnstyled, yes later I can provide a demo for the unstyled one too.

povilass · 2021-02-22T20:08:24Z

@oliviertassinari need to re-run the test because everything runs for me and I updated docs.

oliviertassinari · 2021-02-23T13:07:01Z

@povilass Great job here! This wasn't an easy one :)

povilass · 2021-02-23T13:46:27Z

I call it medium difficulty but with a lot of boilerplate still need to improve in the documentation.

github-actions bot mentioned this pull request Feb 10, 2021

Update snapshots for pr/24857 eps1lon/mui-scripts-incubator#1840

Closed

oliviertassinari mentioned this pull request Feb 11, 2021

Migrate all components to emotion #24405

Closed

oliviertassinari requested changes Feb 11, 2021

View reviewed changes

povilass mentioned this pull request Feb 12, 2021

[Portal] Migrate to unstyled #24890

Merged

1 task

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 12, 2021

This was referenced Feb 16, 2021

[TrapFocus] Migrate to unstyled #24957

Merged

[Backdrop] Migrate to unstyled #24985

Merged

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 19, 2021

oliviertassinari changed the title ~~[Modal] Migrate to emotion~~ [Modal] Migrate to emotion + unstyled Feb 21, 2021

oliviertassinari added the component: modal This is the name of the generic UI component, not the React module! label Feb 21, 2021

oliviertassinari added 6 commits February 21, 2021 23:31

needed since a rebase on HEAD would break without

f532066

shorter

0937b08

shorter

b03dc83

export directly

b1ac8a8

fix ci

aa83be2

my bad, the other are spread on a different component

dff9423

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 21, 2021

oliviertassinari mentioned this pull request Feb 21, 2021

[test] Track bundle size of unstyled components #25047

Merged

fix script error

06d89b1

eps1lon approved these changes Feb 22, 2021

View reviewed changes

povilass added 2 commits February 22, 2021 21:47

demo docs update

6536601

prettier

5c17712

oliviertassinari added 6 commits February 23, 2021 01:52

Showcase unstyled sooner as more relevant for a Modal

db05a07

bring a bit more light to the unstyled sections

0b786d0

fix duplicated id

802106c

create more contrast between styled and unstyled

7d6e490

denser demos

8cf6477

polish

13ffe70

oliviertassinari merged commit c425353 into mui:next Feb 23, 2021

oliviertassinari mentioned this pull request Feb 23, 2021

[core] Remove need to reinject backdrop #25071

Merged

michaldudak mentioned this pull request Feb 27, 2024

Unstyled components progress mui/base-ui#10

Open

35 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Modal] Migrate to emotion + unstyled #24857

[Modal] Migrate to emotion + unstyled #24857

povilass commented Feb 10, 2021 •

edited by oliviertassinari

mui-pr-bot commented Feb 10, 2021 •

edited

povilass commented Feb 10, 2021 •

edited

oliviertassinari commented Feb 10, 2021 •

edited

povilass commented Feb 10, 2021

oliviertassinari left a comment

povilass commented Feb 11, 2021 •

edited

oliviertassinari commented Feb 11, 2021 •

edited

povilass commented Feb 11, 2021

oliviertassinari commented Feb 11, 2021

povilass commented Feb 11, 2021

oliviertassinari commented Feb 11, 2021

povilass commented Feb 11, 2021 •

edited

oliviertassinari commented Feb 11, 2021 •

edited

povilass commented Feb 11, 2021 •

edited

oliviertassinari commented Feb 11, 2021 •

edited

povilass commented Feb 12, 2021 •

edited

oliviertassinari commented Feb 12, 2021

povilass commented Feb 19, 2021

povilass commented Feb 20, 2021

povilass commented Feb 20, 2021 •

edited

oliviertassinari commented Feb 21, 2021

oliviertassinari commented Feb 21, 2021 •

edited

povilass commented Feb 22, 2021

oliviertassinari commented Feb 22, 2021

povilass commented Feb 22, 2021 •

edited

oliviertassinari commented Feb 22, 2021 •

edited

povilass commented Feb 22, 2021 •

edited

povilass commented Feb 22, 2021

oliviertassinari commented Feb 23, 2021

povilass commented Feb 23, 2021

[Modal] Migrate to emotion + unstyled #24857

[Modal] Migrate to emotion + unstyled #24857

Conversation

povilass commented Feb 10, 2021 • edited by oliviertassinari

mui-pr-bot commented Feb 10, 2021 • edited

povilass commented Feb 10, 2021 • edited

oliviertassinari commented Feb 10, 2021 • edited

povilass commented Feb 10, 2021

oliviertassinari left a comment

Choose a reason for hiding this comment

povilass commented Feb 11, 2021 • edited

oliviertassinari commented Feb 11, 2021 • edited

povilass commented Feb 11, 2021

oliviertassinari commented Feb 11, 2021

povilass commented Feb 11, 2021

oliviertassinari commented Feb 11, 2021

povilass commented Feb 11, 2021 • edited

oliviertassinari commented Feb 11, 2021 • edited

povilass commented Feb 11, 2021 • edited

oliviertassinari commented Feb 11, 2021 • edited

povilass commented Feb 12, 2021 • edited

oliviertassinari commented Feb 12, 2021

povilass commented Feb 19, 2021

povilass commented Feb 20, 2021

povilass commented Feb 20, 2021 • edited

oliviertassinari commented Feb 21, 2021

oliviertassinari commented Feb 21, 2021 • edited

povilass commented Feb 22, 2021

oliviertassinari commented Feb 22, 2021

povilass commented Feb 22, 2021 • edited

oliviertassinari commented Feb 22, 2021 • edited

povilass commented Feb 22, 2021 • edited

povilass commented Feb 22, 2021

oliviertassinari commented Feb 23, 2021

povilass commented Feb 23, 2021

povilass commented Feb 10, 2021 •

edited by oliviertassinari

mui-pr-bot commented Feb 10, 2021 •

edited

povilass commented Feb 10, 2021 •

edited

oliviertassinari commented Feb 10, 2021 •

edited

povilass commented Feb 11, 2021 •

edited

oliviertassinari commented Feb 11, 2021 •

edited

povilass commented Feb 11, 2021 •

edited

oliviertassinari commented Feb 11, 2021 •

edited

povilass commented Feb 11, 2021 •

edited

oliviertassinari commented Feb 11, 2021 •

edited

povilass commented Feb 12, 2021 •

edited

povilass commented Feb 20, 2021 •

edited

oliviertassinari commented Feb 21, 2021 •

edited

povilass commented Feb 22, 2021 •

edited

oliviertassinari commented Feb 22, 2021 •

edited

povilass commented Feb 22, 2021 •

edited