[pigment-css][docs] Add small edits on the README #41646
Conversation
danilo-leal
added
docs
Netlify deploy previewhttps://deploy-preview-41646--material-ui.netlify.app/ Bundle size report |
| ```tsx | ||
| interface ButtonProps { | ||
| size?: 'small' | 'large'; | ||
| } | ||
|
|
||
| const Button = styled('button')<ButtonProps>({ |
There was a problem hiding this comment.
So, I started out with the Vite example app and just copying and pasting this code snippet doesn't work because of TypeScript. Given both example apps (Vite & Next.js) are using it by default, I wonder if we shouldn't do something like this? Y'all tell me if this is the best way to do this, as I know there are others. But I guess having these snippets tailored to work seamlessly with the example apps should be the way to go.
This point was what motivated me to open the PR. Depending on what we discuss here, we can change other snippets throughout the README!
There was a problem hiding this comment.
Okay, I just realized (sorry 😅) we have the "Typing props" header a few lines below this code snippet. This sort of solves my point there, but I do still wonder if we shouldn't tailor the docs to TypeScript by default and warn the opposite: instead of saying "If you use TS, add this", say "if you're not using TS, remove this"?
There was a problem hiding this comment.
I would rather see code that works right away when copying and pasting—the user shouldn't need to keep reading further down the page to understand why this doesn't work. You could add a small comment above the TS to let JS users know to remove it.
There was a problem hiding this comment.
Definitely—I can also do the opposite (add a comment about heading over to the TS adaptation) if we're not in the mood to debate making them use TypeScript by default right now. I personally feel like it'd be good to do it, though... the example apps use it, and our own codebase also largely uses it, so I think the consistency would be great there.
There was a problem hiding this comment.
We do have a Typescript section in this README. So we can target ts there. Anyways, there are only 2 points to note for TS (as of now). Rest can just target JS even for TS code.
I am OK with either. One data point to figure out would how many people report issues in our repo specific to writing code for JS and not TS.
| @@ -39,7 +39,9 @@ Pigment CSS is built on top of [WyW-in-JS](https://wyw-in-js.dev/), enabling to | |||
|
|
|||
| ### Start with Next.js | |||
|
|
|||
| Use the following commands to create a new Next.js project with Pigment CSS set up: | |||
| #### Quickstart | |||
There was a problem hiding this comment.
Felt like a title like this one was missing in opposition to the "Manual installation". I know it can sound a bit dumb, but I wondered about this for a couple of seconds before starting 😅 The extra clarity could be worth it.
|
Little bump on this one! :) |
|
@brijeshb42 just want to confirm what you think about making the code snippets use TypeScript by default (and then instruct about removing it if that's not the case) rather than the other way around? |
Little improvement opportunities I realized while testing Pigment CSS on a fresh new repo.