search

mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

Migrate all components to emotion #24405

Closed mnajdova closed 2 years ago

mnajdova commented 3 years ago

As decided in https://github.com/mui-org/material-ui/issues/22342 we need to migrate all the components to the @mateiral-ui/styled-engine (emotion by default) API. We've already converted a dozen components. We are now confident with the approach. We have fixed most of the challenges it creates. This issue will help track the progress of the migration.

For converting a component, you can use as a template any of the already converted components, e.g #24397. We have also this migration guide in case you want to prepare yourself before starting, or in case if something is not working as expected.

This is a useful checklist to have when migrating a component:

General checklist:

Component checklist:

Here is a list of all components with their status:

Note: if you see that the components depends on some other components, then those components need to be converted first, before the component itself can be migrated.

@material-ui/core

@material-ui/lab

Once the migration of the core components is done, we can then migrate all the demos of the documentation to the sx prop (preferred) and styled API, removing all imports of withStyles and makeStyles.

Current progress 169/169 ```jsx Array.from(document.querySelectorAll('.contains-task-list')[2].children) .concat(Array.from(document.querySelectorAll('.contains-task-list')[3].children)).reduce((acc, item) => { if (item.querySelector('input[type="checkbox"]:checked')) { acc.done += 1; } acc.total += 1; return acc; }, { total: 0, done: 0 }); ```
TheRealBenJones commented 3 years ago

Gunna take a shot at Icon for my first commit

natac13 commented 3 years ago

Going to work on Chip

Would it be a good idea as we get to the halfway point, to call the components so no one ends up doing double work?

@kodai3 @vicasas @vinyldarkscratch @xs9627 @duganbrett

natac13 commented 3 years ago

Going to start on Tab

natac13 commented 3 years ago

TabScrollButton next

natac13 commented 3 years ago

Table next

natac13 commented 3 years ago

TableBody

natac13 commented 3 years ago

TableCell next

natac13 commented 3 years ago

TableContainer to finish the night off

mnajdova commented 3 years ago

@eps1lon had a great comment on one of the migration PRs regarding the typings for the classes - https://github.com/mui-org/material-ui/pull/24661#discussion_r565968934

Basically, we do not need to introduce new interfaces for them, we can reuse the already existing ComponentClassKey. With this we will ensure that the typings are in sync and correct in all places.

In the example that I linked, we would basically have the following:

+import { FormHelperTextClassKey } from './FormHelperText';
-export interface FormHelperTextClasses {
-  root: string;
-  error: string;
-  disabled: string;
-  sizeSmall: string;
-  sizeMedium: string;
-  contained: string;
-  focused: string;
-  filled: string;
-  required: string;
-}

-declare const formHelperTextClasses: FormHelperTextClasses;
+declare const formHelperTextClasses: Record<FormHelperTextClassKey, string>;

export function getFormHelperTextUtilityClasses(slot: string): string;

export default formHelperTextClasses;

With this change we can also update the classes prop to include the new available class keys - sizeMedium in this case. Would be great if we can do this in the next PRs, and in the end we can do a revision and update where necessary.

natac13 commented 3 years ago

ToggleButton next for me

natac13 commented 3 years ago

Going to do Step now

ConAntonakos commented 3 years ago

If anyone hasn't grabbed ButtonGroup, I'd love to grab that one to practice learning the internals of this project. 🙂

EDIT: I was too slow! I will grab something else. 😄

natac13 commented 3 years ago

@ConAntonakos Let me know if you need any help.

natac13 commented 3 years ago

For errors with elements not allowed to be children of div in ConformanceV5 testing

render: (node) => {
  const { container, ...rest } = render(<table>{node}</table>);
  return { container: container.firstChild, ...rest };
},

see #24658

natac13 commented 3 years ago

Doing TableFooter

natac13 commented 3 years ago

TableHead

natac13 commented 3 years ago

TableRow to finish the night

natac13 commented 3 years ago

Ill be doing Switch next

natac13 commented 3 years ago

Going to take Autocomplete now

natac13 commented 3 years ago

Checkbox next on the list. I think ill just go down the list now one by one.

oliviertassinari commented 3 years ago

@natac13 Hey, could we hold on a bit? Marija is on maternity leave and I won't have the bandwidth to handle (review) more than 20 components a week (she was doing 50/week). We already have 14 PRs open, If we can review them all by next week, It would already be great. I think that it would be better to get feedback on them, learn how to do the rest more efficiently, thanks :).

oliviertassinari commented 3 years ago

@natac13 Instead, if you could work on making all your open PR have a green CI, (they are all failing currently) it would help :)

natac13 commented 3 years ago

No problem, The Table ones just need #24679 and get rebased to pass. Which I see was done so Ill get on that. the Switch one I think just needs to be re-run Ill have to look at Autocomplete I had a question, yet to be answered And I will fix the checkbox one now.

I think that it would be better to get feedback on them, learn how to do the rest more efficiently,

Not sure what you mean by this

duganbrett commented 3 years ago

I have a branch going for the Select component and the Menu component but will be waiting for open PRs (specifically #24698 and https://github.com/mui-org/material-ui/pull/24688) to resolve before opening new PRs for them.

Also, congrats and good luck to @mnajdova

duganbrett commented 3 years ago

I think that it would be better to get feedback on them, learn how to do the rest more efficiently,

Not sure what you mean by this

I believe he means that he wants time to get feedback on the open PR's before we move on to more components. That way we can apply feedback from the currently open PR's to the future components without having to rework them based on the delayed feedback.

natac13 commented 3 years ago

Yep that make sense. It already happened with comment

natac13 commented 3 years ago

24696 Has a styling error when it comes to first-child it wants first-of-type with SSR

24702, #24684 & #24693 I think just re-run the failing test should work.

I got the rest green. And Ill wait until your ready (and if) you would like me to do any more.

vicasas commented 3 years ago

Personally, I think maybe the overridesResolver and useUtilityClasses utility functions could be in a separate file in order to make the [Component].js file as clean.

vicasas commented 3 years ago

Another thing that I realized in the migration and I don't know if emotion already does it, is the fact of not declaring[Component].displayName = ' ' in the components. Was it being pushed aside for any particular reason? as this was for use in the React developer tools.

oliviertassinari commented 3 years ago

@vicasas We try to have as few files as possible. I believe it's simpler for refactoring and new contributors to follow the logic. The display names for the React devtools are already present.

vicasas commented 3 years ago

@oliviertassinari I understand, thanks for the clarification 😁. That is why personally I tell myself that it seems more fair to separate by responsibilities, but I understand that you do not want to add more files to the core.

oliviertassinari commented 3 years ago

@onpaws opening a new issue would be more appropriate. It's off-topic here. We have never looked into the css prop. The sx is meant to solve the same problem while working with emotion and styled-components indiscriminately. However, we definitely want to be interoperable with this css API of emotion. If you have a reproduction that show that is specific to Material-UI (not a misconfiguration of the environment) then opening a new bug would be great.

onpaws commented 3 years ago

opening a new issue would be more appropriate.

Sorry about that, will delete and move it elsewhere.

Thanks for mentioning about the sx prop - Emotion's React intro was the reason I was trying css in the first place. I'll try to educate myself on this area, and time permitting, would love to contribute to the efforts here. Good luck to you all, feeling excited about this initiative and cheering for you! 🥳

mainfraame commented 3 years ago

@onpaws I am currently using the css prop instead of sx with React v17.0.0. Emotion's css prop requires emotion's jsx runtime, which requires minor modifications to your babel config and tsconfig (if applicable); but, it does work seamlessly.

@babel/preset-react

[
     '@babel/preset-react',
     {  
          runtime: 'automatic', 
          importSource: '@emotion/react'
     }
]

@babel/plugin-transform-typescript <-- if using typescript

[
    '@babel/plugin-transform-typescript',
    {
          isTSX: true,
          jsxPragma: 'transform'
    }
]

@emotion/babel-plugin <-- I believe this is optional

[
    '@emotion/babel-plugin',
    {
        autoLabel: 'dev-only',
        cssPropOptimization: true,
        labelFormat: '[local]',
        sourceMap: true,
    }
]

tsconfig.json

{
    "compilerOptions": {
          "jsx": "preserve",
          "jsxImportSource": "@emotion/react",
          "jsxFactory": "jsx"
     }
}

It's also important to note that css prop is supported on any component that accepts a className prop.

onpaws commented 3 years ago

Emotion's css prop requires emotion's jsx runtime, which requires minor modifications to your babel config and tsconfig (if applicable); but, it does work seamlessly.

Cool, good to hear this should be possible.

Do you use Create React App? Curious if the snippets you posted (thanks!) require ejecting, historically that's been needed but maybe the story has improved in 2021.

(Don't want to spam the thread and distract from the good work going on here -- if you think it ought be possible perhaps we can take it offline @mainfraame ?)

mainfraame commented 3 years ago

@onpaws I did not use create-react-app, but from what I understand you can still modify the babel config: https://devinschulz.com/modify-create-react-apps-babel-configuration-without-ejecting/

The snippets I posted should work for that as well.

oliviertassinari commented 3 years ago

Regarding https://github.com/mui-org/material-ui/issues/24405#issuecomment-770210779

could we hold on a bit?

I have managed to review most of the open pull requests, there is only 3 open, thank you everybody 🙌. 3 are enough for until the end of the week :). I aim to review 1/day. We have 36 components left to migrate, which should allow us to be done before Marija comes back from maternity leave. I think that we could slowly resume the effort. I would be cautious with not having 36 open PRs because the longer a PR is open, the more likely it will have a conflict and we will struggle with a rebase.

On a different note, the migration of the components to emotion is one side of the problem, we will also need to migrate the documentation: #16947 so developers have a single CSS-in-JS solution in their bundle. The slider can be used as a baseline.

natac13 commented 3 years ago

I can take Drawer. Ill try and finish by the weekend.

natac13 commented 3 years ago

Ill take Dialog over the weekend if no one else has started it.

natac13 commented 3 years ago

Hey @oliviertassinari is it alright I do another this weekend? If so I'll take Rating

oliviertassinari commented 3 years ago

@natac13 I won't have the time to review it, but why not

tomasznguyen commented 3 years ago

I was about to start to migrate MenuList. However, I see that there is no styling and slots present in the component. What is the best way to approach MenuList? Introduce a root slot? Move to unstyled? Or is there no need for conversion?

oliviertassinari commented 3 years ago

@tomasznguyen MenuList doesn't have any styles. It doesn't need to be migrated. I'm marking it as done.

kayuapi commented 3 years ago

There is no styling and slots in RadioGroup too. I guess there is no need for conversion too?

mwskwong commented 3 years ago

What about the lab components? I assume they will be migrated as well in the future?

oliviertassinari commented 3 years ago

What about the lab components?

@matthewkwong2 We forgot about them. Updated

oliviertassinari commented 3 years ago

There is no styling and slots in RadioGroup too.

@kayuapi We could potentially add a styled wrapper, but I think that it's for another issue. We haven't yet seen somebody to ask for it. Marked as done ✅.

m4theushw commented 3 years ago

SwipeableDrawer also doesn't have any styles, but it's pending in the list.

vicasas commented 3 years ago

Labs components that correspond to Tab components are not in the site menu or in this issue

oliviertassinari commented 3 years ago

I have removed the "On hold" label as @mnajdova is back. We should have bandwidth now to get to 100% 🔥