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

[RtlProvider] Add component & hook #41241

Merged
merged 10 commits into from
Mar 8, 2024
Merged

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Feb 22, 2024

Fixes #41217

The idea is to add separate context for rtl, outside of the ThemeProvider as the notion of ThemeProvider won't exist in the zero-runtime. The implementation is done in such a way that it is non-breaking when using emotion.

@mui-bot
Copy link

mui-bot commented Feb 22, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 3fafd73

@mnajdova mnajdova added package: system Specific to @mui/system package: pigment-css Specific to @pigment-css/* labels Feb 22, 2024
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

I think we need to scope this PR down to just zero runtime first. How zero runtime is going to support Rtl and then move up to integrate with Material UI.

So I think the first PR would be exposing APIs from zero-runtime, the RtlProvider and the generated styles.

Then the next PR would be integration with Material UI which could likely be the same as createUseThemeProps exported from ../zero-styled.

The structure could look like this:
image

The change should be on Material UI, not System:

// mui-material/src/zero-styled/index.ts
import useTheme from '../styles/useTheme';

export function useRtl() {
  const theme = useTheme();
  return theme.direction === 'rtl';
}



// mui-material/src/Drawer/Drawer;

import { useRtl } from '../zero-styled';

const rtl = useRtl();

@mnajdova
Copy link
Member Author

For useThemeProps, I understand the reasoning for having this double API and switching, as we are getting a benefit for not using context in Material UI, but for rtl, I don't understand why we would complicate that much if we can use common API? I mean it's just context added in the ThemeProvider, it's not necessary to complicate it if we don't gain anything using it.

For supporting rtl in zero-runtime, we can create docs page showing how it can be done, I suppose we would still use the styling plugin cc @brijeshb42 to confirm. This PR just makes sure that in the component's logic we need to have a way to check if the component should be rendered in rtl mode, as the logic depends on it.

@brijeshb42
Copy link
Contributor

@siriwatknp I don't think we can implement useRtl as a processor in zero-runtime similar to what you did with useThemeProps since theme will only have tokens and direction value will come at runtime. Even if theme had theme.direction in the bundler config, that'd still only generate css for that specific value and there won't be a way to change that.

@mnajdova mnajdova marked this pull request as ready for review February 27, 2024 07:32
@siriwatknp
Copy link
Member

@siriwatknp I don't think we can implement useRtl as a processor in zero-runtime similar to what you did with useThemeProps since theme will only have tokens and direction value will come at runtime. Even if theme had theme.direction in the bundler config, that'd still only generate css for that specific value and there won't be a way to change that.

It's not my intention to have rtl in Pigment theme. Pigment still expose the hook and context to get rtl for behavioral logic. Using hook and context from the System works but just a reminder that we are creating dependency between Pigment and System along the way with this approach.

One more thing, I don't see how Pigment handles style interpolation for RTL in this PR. Is it the next step? I think it should be in this PR.

@mnajdova
Copy link
Member Author

mnajdova commented Mar 6, 2024

It's not my intention to have rtl in Pigment theme. Pigment still expose the hook and context to get rtl for behavioral logic. Using hook and context from the System works but just a reminder that we are creating dependency between Pigment and System along the way with this approach.

From what I saw pigment already has a dependency on the system, this is why I decided to add the context there. We can likely long term create some utils package that both system and pigment can depend on.

One more thing, I don't see how Pigment handles style interpolation for RTL in this PR. Is it the next step? I think it should be in this PR.

As I mentioned this will be solved later, we have to revise with Brijesh how we want to tackle this. This is for runtime rendering & different styles based on the state, it's not about transforming the styles.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍

@mnajdova mnajdova merged commit 1e80616 into mui:master Mar 8, 2024
19 checks passed
mnajdova added a commit to mnajdova/material-ui that referenced this pull request Mar 8, 2024
@oliviertassinari
Copy link
Member

as the notion of ThemeProvider won't exist in the zero-runtime

I connected the dot with #43443. It feels like we should remove the RtlProvider component and approach the solution differently.

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/* package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[zero-runtime] Add RTL support
5 participants