Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[system] Make @emotion/* fully supported in the System #33205

Merged
merged 9 commits into from
Aug 1, 2022
Merged

[system] Make @emotion/* fully supported in the System #33205

merged 9 commits into from
Aug 1, 2022

Conversation

garronej
Copy link
Contributor

@garronej garronej commented Jun 18, 2022

Hi,
This PR is a follow-up on #32067 and is addressing #31825.

It is a POC with only the Button component.

The visual test case can be run with:

yarn test:regressions:dev

Then accessing: http://localhost:3000/regression-Button/EmotionCompat#no-dev

With this branch you'll get:
image

With mui/material-ui#master:
image

Summary, for the record, of why I am doing this:

  1. To enable @emotion/react to be used for writing custom-style. See PR#31825.
  2. To make @emotion/css work as expected when user to write custom styles.
  3. Enable TSS to work out of the box with MUI, without user having to explicitly provide a cache and follow specific instructions for SSR.

What is done

When we call clsx() we mind the order in which we provide the arguments.
We make sure to put first the internal styles (lower priority) then, the user-defined styles (higher priority).

We also need to extract classes.root and apply it in the className instead of forwarding it to ButtonBase because we want to enforce that priority wise: internal styles < classes.root < className.

If you give me the green light, I can apply the same changes on every MUI components.

Best regard,

@mui-bot
Copy link

mui-bot commented Jun 18, 2022

Details of bundle changes

Generated by 🚫 dangerJS against e041ef9

@mnajdova mnajdova added bug 🐛 Something doesn't work core Infrastructure work going on behind the scenes labels Jul 5, 2022
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can go ahead and update the components with this change.

@garronej
Copy link
Contributor Author

garronej commented Jul 5, 2022

Ok great @mnajdova,
I will try to get at it this week end.

@garronej
Copy link
Contributor Author

garronej commented Jul 10, 2022

Hi @mnajdova,
I think I am done reordering classes for @mui/material.
I was pleasantly surprised that there weren't that many things to change 😊

packages/mui-material/src/ImageList/ImageList.js Outdated Show resolved Hide resolved
packages/mui-material/src/ImageListItem/ImageListItem.js Outdated Show resolved Hide resolved
packages/mui-material/src/Drawer/Drawer.js Outdated Show resolved Hide resolved
@michaldudak michaldudak merged commit 902b4b0 into mui:master Aug 1, 2022
@garronej
Copy link
Contributor Author

garronej commented Aug 1, 2022

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants