-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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][docs] Update the RTL section on the readme #41576
Conversation
Netlify deploy previewhttps://deploy-preview-41576--material-ui.netlify.app/ Bundle size report |
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.
Thanks for updating.
packages/pigment-css-react/README.md
Outdated
// CSS output option | ||
css: { | ||
// Specify your default direction | ||
defaultDirection: 'rtl', |
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 the generateForBothDir: true,
in this first demo and explain it. I hardly suspect anyone would set the defaultDirection
option without using this option too. The selector option should go in a separate section as it's more or less optional.
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.
That's how it was before, but I felt like we were introducing two things simultaneously: configuring the bundler and what this prop is/why it exists. I was missing the hook to say what is the value of it the way it was before, that's why I'm exploring a separate section 🤔
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.
Yeah. both options will be used together and generateForBothDir
in 99% of the case will be true
.
Also, we already introduced configuring the bundler in the starting section for next and vite. This one focusses more on the css
option.
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.
Gotcha, okay. Take a look again now; I added a title to connect the generated CSS part with the content, so it doesn't look like it is something specifically about the Vite config (unless it is).
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.
mostly nitpicking at this point so I'll tentatively approve!
Just a few writing tweaks: