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

[examples] Add material-ui-pigment-css for Next.js and Vite #43065

Merged
merged 20 commits into from
Aug 13, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jul 25, 2024

There is a problem with pnpm for Vite example (npm and yarn work fine) but it's not a blocker for this PR.

Blocked by mui/pigment-css#193 and mui/pigment-css#191.

@siriwatknp siriwatknp added package: material-ui Specific to @mui/material examples Relating to /examples package: pigment-css Specific to @pigment-css/* labels Jul 25, 2024
@siriwatknp siriwatknp requested review from brijeshb42 and a team July 25, 2024 07:34
Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

Nice job. Did a quick first pass and left some comments.


Visually speaking, some stuff we could improve:

  • Headings in the bullet list look a bit too large.
  • Emojis and headings look misaligned.
  • The bullet list could be moved down so its top is aligned with the main title.

From this

Screenshot 2024-07-25 at 10 43 42

to something like this (quick sketch)

Screenshot 2024-07-25 at 11 15 53

Unrelated to this PR

  • The style attribute is not supported in PigmentGrid
    image
  • Passing the sx prop to PigmentGrid results in the following error in the Next.js demo:
    Screenshot 2024-07-25 at 11 20 02
  • Passing a shortcut to the sx prop results in a TS error, but the style gets applied:
    Screenshot 2024-07-26 at 09 20 41

Comment on lines 80 to 81
Pigment CSS will look through Material UI components used in the project and
extract the styles into plain CSS.
Copy link
Member

Choose a reason for hiding this comment

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

Is this accurate? Wouldn't Pigment also extract styles from styled, css, sx even if they're non-Material UI components?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's accurate, my intention is to point out the integration. We can add more to cover your point, what do you have in mind?

Copy link
Member

Choose a reason for hiding this comment

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

With the current wording some developers might get confused and think that Pigment CSS only works for Material UI components.

@siriwatknp
Copy link
Member Author

Passing the sx prop to PigmentGrid results in the following error in the Next.js demo:

Screenshot 2024-07-25 at 11 20 02

@brijeshb42 seems like @mui/material-pigment-css is not picked up by the plugin.

@mui-bot
Copy link

mui-bot commented Aug 6, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 1c4a562

@siriwatknp siriwatknp added the on hold There is a blocker, we need to wait label Aug 6, 2024
@siriwatknp siriwatknp removed the on hold There is a blocker, we need to wait label Aug 9, 2024
@siriwatknp
Copy link
Member Author

@mui/core ready for a final review.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

examples/material-ui-pigment-css-vite-ts/README.md Outdated Show resolved Hide resolved
examples/material-ui-pigment-css-vite-ts/README.md Outdated Show resolved Hide resolved
@siriwatknp
Copy link
Member Author

I tested the examples on StackBlitz instead of running them locally. The Next.js example throws an error:

This will be fixed by #43237

Co-authored-by: Zeeshan Tamboli <[email protected]>
Signed-off-by: Siriwat K <[email protected]>
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Just realized that both examples can be added to the Example Projects page in the documentation.

@siriwatknp
Copy link
Member Author

@aarongarciah Would you mind do a final review on this? All issues have been resolved.

@aarongarciah
Copy link
Member

@siriwatknp I'm experiencing these errors while trying to run the projects locally. I copied the example folder and ran pnpm i & pnpm dev in both cases.

Next.js

Screenshot 2024-08-12 at 16 50 40

Vite

Screenshot 2024-08-12 at 16 51 58

@o-alexandrov
Copy link
Contributor

@aarongarciah probably because of pnpm issue described in the issue's description:

There is a problem with pnpm for Vite example (npm and yarn work fine) but it's not a blocker for this PR.

@aarongarciah
Copy link
Member

@o-alexandrov ouch! I totally forgot about it, thanks.

Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

@siriwatknp looks cool. I couldn't get the Next.js example to work. I tried with pnpm, npm and yarn v1 with no luck. I receive this error:

image

@siriwatknp
Copy link
Member Author

siriwatknp commented Aug 13, 2024

@siriwatknp looks cool. I couldn't get the Next.js example to work. I tried with pnpm, npm and yarn v1 with no luck. I receive this error:

That issue is fixed by #43237. It has not been released yet but you can test with:

"@mui/material": "https://pkg.csb.dev/mui/material-ui/commit/cd5410b1/@mui/material"

I tested and it works on my end.

Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

@siriwatknp awesome job!

That issue is fixed by #43237. It has not been released yet but you can test with:

Tested with the fixed version and works well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Relating to /examples package: material-ui Specific to @mui/material package: pigment-css Specific to @pigment-css/*
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants