-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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] Create a wrapper package over Pigment CSS #42819
Conversation
adfd849
to
4cc9d46
Compare
@siriwatknp I'll need your help to update any documentation that might be referencing |
4cc9d46
to
3e7b81a
Compare
3e7b81a
to
4235585
Compare
At the moment, it only re-exports stuff from Pigment CSS. But as the Pigment core APIs change, it'll server as the normalizing library that still provides the same API even if Pigment has any breaking change.
4235585
to
da0ee4a
Compare
85bc8e0
to
6ac9e5e
Compare
packages/mui-material-pigment-css/src/Grid/MaterialPigmentGrid.ts
Outdated
Show resolved
Hide resolved
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.
Are we waiting for a release on Pigment CSS to replace the packages?
sourceMap: true, | ||
displayName: true, | ||
overrideContext: (context) => { |
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.
❤️
0a8b7ae
to
a2e16da
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.
Few final small comments.
packages/mui-material-pigment-css/src/Hidden/MaterialPigmentHidden.ts
Outdated
Show resolved
Hide resolved
"moduleResolution": "Node16", | ||
"module": "Node16" |
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.
What's is change for? I am afraid that it might change the build result.
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.
It was added because of usage of exports
field in package.json
in Pigment CSS. Otherwise, the typechecking was not working.
It will not affect the output because we use babel to build the files and tsc
is only used to create the d.ts
declarations.
@brijeshb42 any thought to hide the import { withPigment } from '@pigment-css/nextjs-plugin';
import { extendTheme } from '@mui/material';
export default withPigment({ … },
{
theme: extendTheme(),
}
); |
We can just set this in the |
Okay, I don't see other way but this. |
@brijeshb42 pushed 2 commits:
|
It's just a function, the bundle size increase is really small. Thanks @siriwatknp for pushing the change. |
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.
The code looks good. I left a review on the migration page, but we can iterate in a new PR too, no hard preference from my side.
docs/data/material/migration/migrating-to-v6/migrating-to-pigment-css.md
Outdated
Show resolved
Hide resolved
docs/data/material/migration/migrating-to-v6/migrating-to-pigment-css.md
Outdated
Show resolved
Hide resolved
docs/data/material/migration/migrating-to-v6/migrating-to-pigment-css.md
Outdated
Show resolved
Hide resolved
docs/data/material/migration/migrating-to-v6/migrating-to-pigment-css.md
Outdated
Show resolved
Hide resolved
docs/data/material/migration/migrating-to-v6/migrating-to-pigment-css.md
Outdated
Show resolved
Hide resolved
docs/data/material/migration/migrating-to-v6/migrating-to-pigment-css.md
Outdated
Show resolved
Hide resolved
docs/data/material/migration/migrating-to-v6/migrating-to-pigment-css.md
Outdated
Show resolved
Hide resolved
docs/data/material/migration/migrating-to-v6/migrating-to-pigment-css.md
Outdated
Show resolved
Hide resolved
}); | ||
``` | ||
|
||
Run the following codemod to remove the owner state from the theme: |
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.
I would add before this section to explain the new syntax - variants. Otherwise it seems scary just reading this that the dynamic styles are not supported.
} | ||
} | ||
} | ||
``` |
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.
We are missing the Grid migration guide.
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.
I am not sure if it's related to Pigment migration?
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.
My thought was, if you are migrating to Pigment CSS, you need to use the Pigment CSS grid. We can add it in a follow up PR, let's not block the release for it.
…material-ui into material-pigment-css
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.
👏👏 Well done everyone!
Docs: https://deploy-preview-42819--material-ui.netlify.app/material-ui/migration/migrating-to-pigment-css/
At the moment, it only re-exports stuff from Pigment CSS. But as the
Pigment core APIs change, it'll serve as the normalizing library that
still provides the same API even if Pigment has any breaking change.
This PR depends on the merge and release of this PR.