-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Navigation Component: Add Support for RTL Languages #26334
Conversation
Size Change: +111 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
Paired programmed with @mattwiebe for these code changes |
79113ff
to
6530922
Compare
export const ItemTitleUI = styled( Text )` | ||
margin-right: auto; | ||
text-align: left; | ||
`; |
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'd imagine that if we used SCSS directly, we'd have gained automatic RTL versions. 🤔
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.
Too bad styled-components doesn't have RTL support. I found a snippet that seems to work though: https://codesandbox.io/s/7w2pk6zz1x?file=/src/index.js
It uses https://github.com/cssjanus/cssjanus under the hood
Maybe worth experimenting with it?
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.
Possibly?
I haven't followed the development of G2 and styled components in Gutenberg, so I'm not sure!
Let's involve @ItsJonQ!
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.
We either use something automatic or develop our own little utils. Like we could have {padding('1px 2px 3px 4px')}
function that properly replaces the values if document is in RTL mode.
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.
Great points! We used the pattern we saw here packages/components/src/input-control/styles/input-control-styles.js:275
as an example, but I'm on board for standardization
I'd love to hear Jon Q's input as well.
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.
Haii!!
Too bad styled-components doesn't have RTL support
@david-szabo97 You are correct! There isn't a standardized way to handle RTL within CSS-in-JS yet.
There are several strategies, but the implementation differs between libraries.
(Will share more insight below, but first...)
We either use something automatic or develop our own little utils."
We have one :)
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/utils/rtl.js
It's a very naive (and tedious) way to do it.
I would love for that process to be automated.
☀️ Thoughts
For run-time compiled styles (e.g. Emotion/Styled components), you're able to tap into the inner CSS compiler (Stylis) and add custom plugins. This "middleware" layer allows you to transform CSS styles just before they're rendered.
Naturally, this is where folks would add plugins to handle things like RTL (which is what G2 Components does).
There's a couple more details to this, which I'll add in the ending notes:
The go-to RTL (JS) library is CSSJanus. Unfortunately, it is licensed under Apache 2.0, which is currently not compatible with Gutenberg's license.
(That means that G2's RTL implementation will need to be removed if/when integrated into Gutenberg)
My regex skills aren't strong enough to create my own RTL solution. (I tried).
Hopefully this helps!
📝 Notes
- RTL in CSS-in-JS can be achieved by tapping into the plugin/middleware layer for Emotion
- A stylis RTL plugin can be added
- This needs to be added to a custom Emotion cache
- For Gutenberg's usage of emotion, (using
styled
), this can be achieved by creating and using an Emotion cache provider. All styled components would render inside. Something like this.
Bonus: G2 Components can achieve this without a provider due to it's custom Emotion implementation
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 the insight here Jon! Much appreciated.
Working well for me 👍 |
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.
LGTM! 🚢
b7d3b38
to
675f4bc
Compare
margin-left: ${ ( props ) => ( props.isRTL ? '0' : '8px' ) }; | ||
margin-right: ${ ( props ) => ( props.isRTL ? '8px' : '0' ) }; | ||
margin-left: ${ ( props ) => ( props.isRTL ? '0' : space( 1 ) ) }; | ||
margin-right: ${ ( props ) => ( props.isRTL ? space( 1 ) : '0' ) }; | ||
display: inline-flex; | ||
padding: 4px 12px; |
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.
Can we use space
here too?
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.
Ah my mistake David. I didn't see these comments because I was focused on getting the e2e tests to pass, but I agree that they should be switched to space
.
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'm going to open up a quick PR to update all pixel values to use space
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.
@@ -126,7 +129,9 @@ export const MenuTitleSearchUI = styled.div` | |||
export const GroupTitleUI = styled( Text )` | |||
margin-top: 8px; |
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.
Can we use space
here too?
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.
LGTM!
@david-szabo97 makes valid points, but are ultimately unrelated to this PR (and related to whoever wrote the component in the first place 😷 ), so they shouldn't be blockers here, and can be addressed in a follow-up, or even whenever we actually do a proper design review of the whole component.
2f3a12c
to
70affaa
Compare
Thanks for the reviews! I'm rebasing master periodically, and will merge this PR whenever the Github CI checks aren't imploding 💥 |
70affaa
to
a129864
Compare
The |
Paired programmed with @mattwiebe for these code changes
Description
There are several arrow symbol and navigation items that need to be adjusted (or even moved) according to the RTL directions.
How has this been tested?
Site Editor
Storybook
yarn storybook:dev
Screenshots
Before
After
Types of changes
Fixes #25997
Checklist: