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

[material-ui][Alert] Convert to support zero runtime #41230

Merged
merged 27 commits into from
Mar 4, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Feb 22, 2024

Waiting for #41317

@@ -0,0 +1,45 @@
import { styled } from '@mui/zero-runtime';
import BasicAlerts from '../../../../../docs/data/material/components/alert/BasicAlerts';
Copy link
Member Author

Choose a reason for hiding this comment

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

At first, I tried to pull all the demos programmatically and render them with lazy loading but not successful, so I'll just go with importing manually and revisit this when I have more time.

Comment on lines 3 to 5
import { AppRouterCacheProvider } from '@mui/material-nextjs/v14-appRouter';
import { ThemeProvider } from '@mui/material/styles';
import theme from './theme';
Copy link
Member Author

Choose a reason for hiding this comment

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

Add this to test that emotion still works with zero-runtime.

@siriwatknp siriwatknp changed the title [material-ui] Enable Alert to support static extraction [material-ui][Alert] Convert to support zero runtime Feb 22, 2024
@siriwatknp siriwatknp added package: material-ui Specific to @mui/material component: alert This is the name of the generic UI component, not the React module! labels Feb 22, 2024
@mui-bot
Copy link

mui-bot commented Feb 22, 2024

Netlify deploy preview

https://deploy-preview-41230--material-ui.netlify.app/

@material-ui/core: parsed: +0.06% , gzip: +0.07%
@material-ui/lab: parsed: -0.05% 😍, gzip: +0.61%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against df00cea

@mnajdova
Copy link
Member

@siriwatknp let's create separate PR for setting up how we test the components. We already have the Badge, Autocomplete, Avatar migrated to use the variants API, it will be easier to review. We can clean this PR later to just show the changes that developers migrating the components will need to do.

@mnajdova
Copy link
Member

Ultimately I want to use the Slider as a template, because it has an example of both rtl and variants. We will need to solve #41217 first.

@siriwatknp
Copy link
Member Author

@siriwatknp let's create separate PR for setting up how we test the components. We already have the Badge, Autocomplete, Avatar migrated to use the variants API, it will be easier to review. We can clean this PR later to just show the changes that developers migrating the components will need to do.

I can open another PR to tackle the migration setup first. My goal is to render the Material UI component from @mui/material, not the next-app components folder.

@mnajdova
Copy link
Member

I can open another PR to tackle the migration setup first. My goal is to render the Material UI component from @mui/material, not the next-app components folder.

Yeah, that's great. I will update my PRs once we have this 👌

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 27, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 28, 2024
@@ -3,6 +3,6 @@
"version": "0.0.1",
"private": true,
"dependencies": {
"@mui/zero-runtime": "file:../../packages/zero-runtime/build"
"@mui/zero-runtime": "file:../../packages/zero-runtime"
Copy link
Member Author

Choose a reason for hiding this comment

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

Mistake from other PR.

Comment on lines 4 to 3
margin: 0;
}
@layer reset, mui;

html,
body {
max-width: 100vw;
overflow-x: hidden;
}
@layer reset {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is to make the emotion overrides the globals.css, not related to zero-runtime.

@@ -0,0 +1,44 @@
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Copy link
Member Author

Choose a reason for hiding this comment

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

Add an index page that links to each demo page.

@@ -0,0 +1,72 @@
'use client';
Copy link
Member Author

Choose a reason for hiding this comment

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

Generated from zero-render-mui-demos.mjs script.

@@ -169,6 +182,7 @@ const Alert = React.forwardRef(function Alert(inProps, ref) {
color,
severity,
variant,
colorSeverity: color || severity,
Copy link
Member Author

@siriwatknp siriwatknp Feb 28, 2024

Choose a reason for hiding this comment

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

This is required because we can't do:

...Object.entries(theme.palette)
    .filter(([, value]) => value.main)
    .map(([color]) => ({
      // this will throw error because a function cannot read variable `color`.
      props: ({ ownerState }) => color === (ownerState.color || ownerState.severity) && ownerState.variant === 'outlined',
      style: {
        color: theme.vars
          ? theme.vars.palette.Alert[`${color}Color`]
          : getColor(theme.palette[color].light, 0.6),
        border: `1px solid ${(theme.vars || theme).palette[color].light}`,
        [`& .${alertClasses.icon}`]: theme.vars
          ? { color: theme.vars.palette.Alert[`${color}IconColor`] }
          : {
              color: theme.palette[color].main,
            },
      },
    })),

So (ownerState.color || ownerState.severity) needs to be resolved before passing to ownerState.

@siriwatknp
Copy link
Member Author

@brijeshb42 @mnajdova It's ready for review.

Copy link
Contributor

@brijeshb42 brijeshb42 left a comment

Choose a reason for hiding this comment

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

LGTM

@siriwatknp siriwatknp added the on hold There is a blocker, we need to wait label Mar 1, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 1, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 4, 2024
@siriwatknp siriwatknp merged commit e93d4ac into mui:master Mar 4, 2024
19 checks passed
@alippai
Copy link

alippai commented Mar 5, 2024

Is there a preliminary benchmark in terms of asset sizes, build times or render timings?
This is in prod since v5.15.12, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: alert This is the name of the generic UI component, not the React module! on hold There is a blocker, we need to wait package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants