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][docs] Update the RTL section on the readme #41576

Merged
merged 8 commits into from Mar 21, 2024

Conversation

danilo-leal
Copy link
Contributor

Just a few writing tweaks:

  • "RTL" might be a well-known acronym, but I don't think it hurts to start first with what it means and then introduce the acronym later on after we have mentioned its meaning.
  • Moving this section above the "How-to guides". It feels like a feature that'd be a sibling to "Theming"?

@danilo-leal danilo-leal added docs Improvements or additions to the documentation package: pigment-css Specific to @pigment-css/* labels Mar 20, 2024
@danilo-leal danilo-leal self-assigned this Mar 20, 2024
@mui-bot
Copy link

mui-bot commented Mar 20, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 2626b1c

Copy link
Contributor

@brijeshb42 brijeshb42 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 updating.

packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
packages/pigment-css-react/README.md Show resolved Hide resolved
packages/pigment-css-react/README.md Show resolved Hide resolved
// CSS output option
css: {
// Specify your default direction
defaultDirection: 'rtl',
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 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.

Copy link
Contributor Author

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 🤔

Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Member

@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.

mostly nitpicking at this point so I'll tentatively approve!

packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
@danilo-leal danilo-leal enabled auto-merge (squash) March 21, 2024 18:31
@danilo-leal danilo-leal merged commit 822a7e6 into mui:next Mar 21, 2024
18 checks passed
@danilo-leal danilo-leal deleted the update-pigment-readme-rtl branch March 21, 2024 22:57
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.

None yet

5 participants