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.

Sign up for GitHub

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

Jump to bottom

[material-ui] Migrate components to support CSS extraction #42001

Merged
merged 66 commits into from
May 6, 2024
Merged

[material-ui] Migrate components to support CSS extraction #42001

merged 66 commits into from
May 6, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Apr 22, 2024 •
edited
Loading

Use styled-v6 codemod + edge case manual adjustment for the rest of the components in #41273

@siriwatknp siriwatknp added component: dialog This is the name of the generic UI component, not the React module! component: drawer This is the name of the generic UI component, not the React module! component: Icon The React component. component: menu This is the name of the generic UI component, not the React module! component: Paper This is the name of the generic UI component, not the React module! component: snackbar This is the name of the generic UI component, not the React module! component: tabs This is the name of the generic UI component, not the React module! component: tooltip This is the name of the generic UI component, not the React module! component: speed dial This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Apr 22, 2024
siriwatknp
siriwatknp commented May 3, 2024
View reviewed changes
apps/pigment-css-next-app/next.config.js
@@ -104,6 +111,26 @@ const pigmentOptions = {
transformLibraries: ['local-ui-lib'],
sourceMap: true,
displayName: true,
overrideContext: (context) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

A workaround for mui/pigment-css#10

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this here fixes the issue? Should this then be made part of the library itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I can confirm that this prevents the error. I added here just to be a workaround but if you think it should be a part of Pigment, I am happy to add it. However, I think it should be a separate package for integrating with MUI system, not a part of nextjs/vite plugin.

Copy link
Member

Choose a reason for hiding this comment

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

If this would be required for every project that uses Material UI and Pigment CSS, we should make it part of the library. Do we know why it is required tough? Maybe we are fixing a symptom, not the problem

Copy link
Contributor

@o-alexandrov o-alexandrov May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

probably related issue: mui/pigment-css#8

@siriwatknp siriwatknp requested a review from DiegoAndai May 3, 2024 11:19
@siriwatknp siriwatknp added component: LoadingButton The React component. component: Typography The React component. component: link This is the name of the generic UI component, not the React module! labels May 3, 2024
DiegoAndai
DiegoAndai approved these changes May 3, 2024
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Amazing job with the codemod Jun, congrats 🎉

I just have some comments but they're not blockers, LGTM

packages/mui-material/src/FilledInput/FilledInput.js Show resolved Hide resolved
packages/mui-material/src/Select/SelectInput.js Show resolved Hide resolved
packages/mui-material/src/Typography/Typography.js Outdated Show resolved Hide resolved
mnajdova
mnajdova reviewed May 6, 2024
View reviewed changes
packages/mui-material/src/Checkbox/Checkbox.js
@@ -117,6 +117,7 @@ const Checkbox = React.forwardRef(function Checkbox(inProps, ref) {
indeterminateIcon: indeterminateIconProp = defaultIndeterminateIcon,
inputProps,
size = 'medium',
disableRipple = false,
Copy link
Member

Choose a reason for hiding this comment

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

What's with this change?

Copy link
Member Author

@siriwatknp siriwatknp May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Otherwise, variants won't work because disabled=undefined

variants: [
  { props: { color: 'default', disableRipple: false } }, style: { … } },
]

apps/pigment-css-next-app/next.config.js
@@ -104,6 +111,26 @@ const pigmentOptions = {
transformLibraries: ['local-ui-lib'],
sourceMap: true,
displayName: true,
overrideContext: (context) => {
Copy link
Member

Choose a reason for hiding this comment

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

If this would be required for every project that uses Material UI and Pigment CSS, we should make it part of the library. Do we know why it is required tough? Maybe we are fixing a symptom, not the problem

packages/mui-material/src/Paper/Paper.js Outdated
@@ -107,6 +113,22 @@ const Paper = React.forwardRef(function Paper(inProps, ref) {
className={clsx(classes.root, className)}
ref={ref}
{...other}
style={{
...other.style,
Copy link
Member

Choose a reason for hiding this comment

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

This should come last, props' style should override the predefined CSS vars if needed.

packages/mui-material/src/Typography/Typography.js Outdated
@@ -137,6 +175,10 @@ const Typography = React.forwardRef(function Typography(inProps, ref) {
className={clsx(classes.root, className)}
{...other}
ownerState={ownerState}
style={{
...other.style,
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this should come last.

@siriwatknp siriwatknp enabled auto-merge (squash) May 6, 2024 11:22
@siriwatknp siriwatknp merged commit ad85002 into mui:next May 6, 2024
22 checks passed
@DiegoAndai DiegoAndai mentioned this pull request May 22, 2024
Closed
oliviertassinari
oliviertassinari reviewed Jun 23, 2024
View reviewed changes
packages/mui-material/src/Paper/Paper.js
@@ -107,6 +113,22 @@ const Paper = React.forwardRef(function Paper(inProps, ref) {
className={clsx(classes.root, className)}
ref={ref}
{...other}
style={{
...(variant === 'elevation' && {
'--Paper-shadow': (theme.vars || theme).shadows[elevation],
Copy link
Member

@oliviertassinari oliviertassinari Jun 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We should prefix this variable no? --Paper-shadow feels like one that would easily conflict. If we match the values in the theme:

Suggested change
'--Paper-shadow': (theme.vars || theme).shadows[elevation],
'--MuiPaper-shadow': (theme.vars || theme).shadows[elevation],

This was referenced Oct 11, 2024
Open
Closed
@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@o-alexandrov o-alexandrov o-alexandrov left review comments

@mnajdova mnajdova mnajdova left review comments

@oliviertassinari oliviertassinari oliviertassinari left review comments

@lhilgert9 lhilgert9 lhilgert9 requested changes

@DiegoAndai DiegoAndai DiegoAndai approved these changes

@brijeshb42 brijeshb42 Awaiting requested review from brijeshb42

Assignees
No one assigned
Labels
component: dialog This is the name of the generic UI component, not the React module! component: drawer This is the name of the generic UI component, not the React module! component: Icon The React component. component: link This is the name of the generic UI component, not the React module! component: LoadingButton The React component. component: menu This is the name of the generic UI component, not the React module! component: Paper This is the name of the generic UI component, not the React module! component: skeleton This is the name of the generic UI component, not the React module! component: snackbar This is the name of the generic UI component, not the React module! component: speed dial This is the name of the generic UI component, not the React module! component: TablePagination The React component. component: tabs This is the name of the generic UI component, not the React module! component: text field This is the name of the generic UI component, not the React module! component: tooltip This is the name of the generic UI component, not the React module! component: Typography The React component. package: material-ui Specific to @mui/material v6.x
Projects
Material UI
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@siriwatknp @mui-bot @brijeshb42 @oliviertassinari @mnajdova @o-alexandrov @DiegoAndai @lhilgert9