-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[pigment-css] Rename zero-runtime to pigmentcss #41317
Conversation
Netlify deploy previewhttps://deploy-preview-41317--material-ui.netlify.app/ Bundle size report |
2b4c993
to
a82c62d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brijeshb42 I wonder if we should change apps/*
to pigment-apps/*
? I think the folder is really specific to pigment.
Right now, it is. But could be a generic apps directory in future where non pigment apps also live. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Pre-approved. Please recheck the failed CI before merging.
@@ -24,7 +24,7 @@ module.exports = { | |||
'@mui/material-next': path.resolve(__dirname, './packages/mui-material-next/src'), | |||
'@mui/material-nextjs': path.resolve(__dirname, './packages/mui-material-nextjs/src'), | |||
'@mui/joy': path.resolve(__dirname, './packages/mui-joy/src'), | |||
'@mui/zero-runtime': path.resolve(__dirname, './packages/zero-runtime/src'), | |||
'@pigmentcss/react': path.resolve(__dirname, './packages/pigment-react/src'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we continue with the existing convention: have the folders exactly match the npm package name? This will avoid confusion, the length of it doesn't seem to matter. It's also one less thing we need to decide on.
'@pigmentcss/react': path.resolve(__dirname, './packages/pigment-react/src'), | |
'@pigmentcss/react': path.resolve(__dirname, './packages/pigmentcss-react/src'), |
(also true if moved to https://github.com/mui/mui-system)
@@ -1,4 +1,4 @@ | |||
import { keyframes, styled } from '@mui/zero-runtime'; | |||
import { keyframes, styled } from '@pigmentcss/react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I read this code, I read the project brand name is PigmentCSS but it's not what https://www.notion.so/mui-org/product-System-vs-zero-runtime-brand-and-vision-fbe3f1fe33e94657b30efef24d82c835 says.
The npm package name convention is kebab case so if our brand name is Pigment CSS this would correctly communicate it:
import { keyframes, styled } from '@pigmentcss/react'; | |
import { keyframes, styled } from '@pigment-css/react'; |
I'm moving this to https://www.notion.so/mui-org/system-npm-package-name-convention-5b0b75983d934c70aab12ae671198d8c. We need to change something, either the brand name of the package name, It doesn't seem coherent right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I read this code, I read the project brand name is PigmentCSS but it's not what https://www.notion.so/mui-org/product-System-vs-zero-runtime-brand-and-vision-fbe3f1fe33e94657b30efef24d82c835 says
I am fine with this.
I'm moving this to https://www.notion.so/mui-org/system-npm-package-name-convention-5b0b75983d934c70aab12ae671198d8c. We need to change something, either the brand name of the package name, It doesn't seem coherent right now.
I think @pigmentcss
works just fine.
We missed the non breaking space in the name in this PR. I opened #41438 to fix this. |
This PR mainly renames all the zero-runtime references to Pigment CSS and package names.