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

[pigment-css] Create a wrapper package over Pigment CSS #42819

Merged
merged 24 commits into from
Jul 10, 2024

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented Jul 2, 2024

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.

@brijeshb42 brijeshb42 added the package: pigment-css Specific to @pigment-css/* label Jul 2, 2024
@brijeshb42
Copy link
Contributor Author

@siriwatknp I'll need your help to update any documentation that might be referencing @pigment-css/react.

@mui-bot
Copy link

mui-bot commented Jul 2, 2024

apps/pigment-css-vite-app/package.json Outdated Show resolved Hide resolved
packages/mui-pigment-css/package.json Outdated Show resolved Hide resolved
@brijeshb42 brijeshb42 changed the title [pigment-css] A new package that is a wrapper over Pigment CSS [pigment-css] Create a wrapper package over Pigment CSS Jul 2, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 9, 2024
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.
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 9, 2024
@brijeshb42 brijeshb42 requested a review from mnajdova July 9, 2024 11:35
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.

Are we waiting for a release on Pigment CSS to replace the packages?

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.

❤️

apps/pigment-css-vite-app/src/pages/material-ui/index.tsx Outdated Show resolved Hide resolved
apps/pigment-css-vite-app/package.json Outdated Show resolved Hide resolved
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.

Few final small comments.

apps/pigment-css-vite-app/vite.config.ts Outdated Show resolved Hide resolved
apps/pigment-css-vite-app/vite.config.ts Show resolved Hide resolved
Comment on lines 12 to 13
"moduleResolution": "Node16",
"module": "Node16"
Copy link
Member

@siriwatknp siriwatknp Jul 10, 2024

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.

Copy link
Contributor Author

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.

@siriwatknp
Copy link
Member

siriwatknp commented Jul 10, 2024

@brijeshb42 any thought to hide the .toRuntimeSource in the configuration? so user can just do this:

import { withPigment } from '@pigment-css/nextjs-plugin';
import { extendTheme } from '@mui/material';

export default withPigment({},
  {
    theme: extendTheme(),
  }
);

@brijeshb42
Copy link
Contributor Author

@brijeshb42 any thought to hide the .toRuntimeSource in the configuration? so user can just do this:

We can just set this in the extendTheme definition inside @mui/material to simplify. This will affect bundle size when not using Pigment but the change should be insignificant.

@siriwatknp
Copy link
Member

@brijeshb42 any thought to hide the .toRuntimeSource in the configuration? so user can just do this:

We can just set this in the extendTheme definition inside @mui/material to simplify. This will affect bundle size when not using Pigment but the change should be insignificant.

Okay, I don't see other way but this.

@siriwatknp
Copy link
Member

@brijeshb42 pushed 2 commits:

  • rename components to follow convention
  • add toRuntimeSource to extendTheme.

@mnajdova
Copy link
Member

We can just set this in the extendTheme definition inside @mui/material to simplify. This will affect bundle size when not using Pigment but the change should be insignificant.

It's just a function, the bundle size increase is really small. Thanks @siriwatknp for pushing the change.

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.

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.

});
```

Run the following codemod to remove the owner state from the theme:
Copy link
Member

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.

}
}
}
```
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👏👏 Well done everyone!

@brijeshb42 brijeshb42 merged commit 63878a5 into mui:next Jul 10, 2024
23 checks passed
@brijeshb42 brijeshb42 deleted the material-pigment-css branch July 10, 2024 14:59
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: pigment-css Specific to @pigment-css/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants