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][react] RTL Support #41570

Merged
merged 3 commits into from
Mar 20, 2024
Merged

[pigment-css][react] RTL Support #41570

merged 3 commits into from
Mar 20, 2024

Conversation

brijeshb42
Copy link
Contributor

  • Re-export RtlProvider from @mui/system
  • Also implements built-in RTL support. See the added test files.

Note: There is one optimization that we can pick-up later is to only add properties that actually changed in the output from ltr to rtl, ie, for:

.class {
  display: block;
  margin-left: 20px;
}

The output should not include the display property since it's common in both.

@brijeshb42 brijeshb42 added the package: pigment-css Specific to @pigment-css/* label Mar 20, 2024
@mui-bot
Copy link

mui-bot commented Mar 20, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 188300c

Also implements built-in RTL support. See the added test files.
@mnajdova
Copy link
Member

Note: There is one optimization that we can pick-up later is to only add properties that actually changed in the output from ltr to rtl

Agree, we should add it as an item for the stable milestone.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Tests looks good to me 👍

packages/pigment-css-react/README.md Outdated Show resolved Hide resolved
@siriwatknp
Copy link
Member

siriwatknp commented Mar 20, 2024

👍 Great to see this coming!

For API, I think it can be refined a bit. Here is what I can think of RTL usage start with the simplest need:

  1. The website is only RTL (maybe for Arabic users only), the config should look like:
{
  theme: ,
  direction: {
    defaultDirection: 'rtl',
  },
}

the output CSS should switch to RTL

  1. The website supports both directions with ltr or rtl as a default:
{
  theme: ,
  direction: {
    output: ['ltr', 'rtl'],
    defaultDirection: 'rtl',
  },
}

The output CSS contains rtl as default and ltr selector.

  1. Need to configure the selector:
{
  theme: ,
  direction: {
    output: ['ltr', 'rtl'],
    defaultDirection: 'rtl',
    getDirSelector(dir: string) {
      // return your own selector that you'd like to use.
      return `:dir(${dir})`;
    }
  },
}
  1. This is an edge case but it could happen. Don't need default output CSS, styles should be targeted with dir selector:
{
  theme: ,
  direction: {
    output: ['ltr', 'rtl'], // since `defaultDirection` is not specified, there is no default CSS output
  },
}

The CSS output will be:

[dir="ltr"] .hashed { … };
[dir="rtl"] .hashed { … };

@brijeshb42
Copy link
Contributor Author

@siriwatknp One point to note here is that if your app only supports one direction (either ltr or rtl), then you'd also write your CSS that way, eg, for ltr, you may write:

css`
  margin-left: 10px;
  margin-right: 20px;
`

and same for rtl would be

css`
  margin-left: 20px;
  margin-right: 10px;
`

I don't think there would be a case where you are only supporting rtl but authoring CSS as ltr. That's why the options are defaultDirection to let the plugin know your default css authoring direction so that when you pass generateForBothDir as true, it can generate css for the other direction.

Also, if you are only supporting one direction, then there is nothing extra in the plugin configuration that you need to do.
The dir selector in the output css in only for wrapping the non-default direction. No need to do both [dir=ltr] and [dir=rtl].

@brijeshb42 brijeshb42 merged commit 2e84bea into mui:next Mar 20, 2024
19 checks passed
@brijeshb42 brijeshb42 deleted the pigment/rtl branch March 20, 2024 12:13
@samuelsycamore
Copy link
Contributor

samuelsycamore commented Mar 20, 2024

@brijeshb42 please tag me for a copyediting review when adding documentation. 🙏 I'll open a new PR to iterate on these changes. Edit: @danilo-leal beat me to it 😁 #41576

pluvio72 pushed a commit to pluvio72/material-ui that referenced this pull request Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: pigment-css Specific to @pigment-css/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants