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

[docs][pigment-css] Add guide for Pigment CSS quickstart #43395

Merged
merged 21 commits into from
Sep 10, 2024

Conversation

alelthomas
Copy link
Contributor

@alelthomas alelthomas commented Aug 21, 2024

Added to material-ui/guides for now, but let me know where it should live in the meantime

Preview: https://deploy-preview-43395--material-ui.netlify.app/material-ui/experimental-api/pigment-css/

@mui-bot
Copy link

mui-bot commented Aug 21, 2024

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 0c3c3d7

@alelthomas alelthomas changed the title [docs] Add guide for Pigment CSS quickstart [docs][pigment-css] Add guide for Pigment CSS quickstart Aug 21, 2024
@samuelsycamore samuelsycamore added docs Improvements or additions to the documentation package: pigment-css Specific to @pigment-css/* labels Aug 21, 2024
@samuelsycamore
Copy link
Contributor

I'm curious to hear @mnajdova 's thoughts on where this doc should live!

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @alelthomas!

About the failing test_static test, running pnpm prettier will fix it, but I recommend you set up auto-format on save on your editor so you don't have to do it manually 😊. Let me know if you need help with that.

About the placing of the guide, How-to guides is what makes the most sense to me, but if the team prefers to move it to Experimental APIs I'm not against that.

Besides that, a couple of comments:

docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
@stunaz
Copy link
Contributor

stunaz commented Aug 22, 2024

It would be great to add a working sandbox

@samuelsycamore
Copy link
Contributor

About the placing of the guide, How-to guides is what makes the most sense to me, but if the team prefers to move it to Experimental APIs I'm not against that.

As I think about it more, Experimental APIs makes the most sense to me. Pigment CSS is still in the "experimental" stage of development after all, at least to some degree. Eventually it should have its own standalone docs space and this doc would be part of the Getting Started sequence. I think putting it in the Experimental APIs section helps to signal that the content is subject to change and could be relocated at a later date. If we want to publish multiple Pigment docs in the Material UI space, we could add a Pigment CSS subheader in the Experimental APIs section as we've done for CSS Theme Variables.

My biggest worry re: publishing Pigment docs in the Material UI space is that this will signal that Pigment is "merely" a Material UI utility, rather than a standalone product in its own right. I suppose this is part of a larger, ongoing conversation about the GTM strategy for Pigment CSS, which seems unclear to me at this stage. But I digress. In any case, when it's time to launch Pigment CSS v1, I would expect it to have its own dedicated docs space, and so I think we should organize "interim" docs like this one with that in mind.

@DiegoAndai
Copy link
Member

DiegoAndai commented Aug 22, 2024

As I think about it more, Experimental APIs makes the most sense to me

That's good for me. Let's move it to Experimental APIs 👍🏼 @alelthomas

My biggest worry re: publishing Pigment docs in the Material UI space is that this will signal that Pigment is "merely" a Material UI utility, rather than a standalone product in its own right.

We won't publish Pigment docs in the Material UI space. This is a user-friendly guide for Material UI users who want to check out Pigment CSS, as without it we might get little feedback for v1 (the migration guide is more focused on migration but not using Pigment). When Pigment has its docs and guides, we can point them there and/or move this guide.

Copy link
Contributor

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

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

Great piece @alelthomas! This review is mostly about copyediting.

One thing I'd really like to see in the next draft is more specific details about why/when to use each of the utilities. You started to do this with the theming section—themes are for applying consistent values across an entire app. It would be really helpful to add a similar description for the other APIs as well, so users know which one to pick for any given use case (e.g., "the sx prop is best for one-off styles when you need to customize a single instance of a component.")

docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
docs/data/material/guides/pigment-css/pigment-css.md Outdated Show resolved Hide resolved
@alelthomas
Copy link
Contributor Author

Made a bunch of small changes based on feedback - let me know what you think @samuelsycamore @DiegoAndai

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

LGTM 😊
Thanks @alelthomas

Copy link
Contributor

@samuelsycamore samuelsycamore left a comment

Choose a reason for hiding this comment

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

This is looking really good! Just some small copyediting suggestions on the newer bits here, otherwise I'd say it's about ready to publish.

@siriwatknp
Copy link
Member

Here is my suggestion for the Next.js and Vite instructions, you can make them as tabs, see alelthomas#1.

It's similar to package manager installation. If you pick Vite, all of the tabs will switch to Vite.

Screen.Recording.2567-09-04.at.10.10.44.mov

@alelthomas alelthomas merged commit 1ad4c8d into mui:master Sep 10, 2024
22 checks passed
@alelthomas alelthomas deleted the pigment-css-quickstart branch September 10, 2024 16:11
Comment on lines +298 to +300
```js
extendTheme({
colorSchemes: {
Copy link
Member

Choose a reason for hiding this comment

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

Wrong pragma and wrong indentation, should be:

Suggested change
```js
extendTheme({
colorSchemes: {
```diff
extendTheme({
colorSchemes: {


If you use the `sx` prop on an HTML element, you'll need to augment the `HTMLAttributes` interface:

```js
Copy link
Member

@oliviertassinari oliviertassinari Sep 14, 2024

Choose a reason for hiding this comment

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

Should get the correct color highlighting:

Suggested change
```js
```ts

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 18, 2024

Fixed those small issues in #43812.

Now, I find the docs misleading. We have the Pigment CSS docs (not the Material UI integration of Pigment CSS) in the documentation of Material UI: https://mui.com/material-ui/experimental-api/pigment-css/. Please no.

@alelthomas How about we?

  1. Move all content that is about Pigment CSS to https://github.com/mui/pigment-css/blob/master/README.md (most of the page it seems today).
  2. Remove all content that is about Pigment CSS only from https://mui.com/material-ui/experimental-api/pigment-css/. I think it's critical that we clearly separate the concerns. An example of what success looks like: Shadcn UI migrating to Pigment CSS.
  3. Use https://mui.com/material-ui/experimental-api/pigment-css/ to host docs that is Material UI specific on how to use Pigment CSS. I would dream of a world where this page is useless, as Shadcn UI has nothing about how to use Tailwind CSS, it's naturally extended it. But still, I gues we will need content there, we are not there yet.

Actually, I'm also confused by https://mui.com/material-ui/migration/migrating-to-pigment-css/. It is located in a place that makes it seem like it's about Material UI v5 to Material UI v6. But it's about Emotion to Pigment CSS. How about we move this content? To:

cc @aarongarciah

@oliviertassinari
Copy link
Member

I have updated mui/pigment-css#73 to reflect the direction suggested in #43395 (comment).

Comment on lines +47 to +48
npm install @pigment-css/react@next
npm install --save-dev @pigment-css/vite-plugin@next
Copy link
Member

Choose a reason for hiding this comment

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

Fixing those in #44353

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation package: pigment-css Specific to @pigment-css/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants