[RNMobile] Add missing RTL support for some mobile components#21502
[RNMobile] Add missing RTL support for some mobile components#21502
Conversation
|
Size Change: -3.61 kB (0%) Total Size: 821 kB
ℹ️ View Unchanged
|
etoledom
left a comment
There was a problem hiding this comment.
Thank you for this PR @jbinda !
The gallery arrows now look and works great on RTL! 🎉
There's still a problem with undo/redo: Since it's a representation of time over the X axis, we need to respect the past (undo) going to the left and the future (redo) going to the right. The directionality representation of time doesn't change on RTL languages.
I also left a comment with an alternative implementation idea. I'd like to know your opinion about it!
| select( 'core/editor' ).getEditorSettings().richEditingEnabled, | ||
| isTextModeEnabled: | ||
| select( 'core/edit-post' ).getEditorMode() === 'text', | ||
| isRTL: I18nManager.isRTL, |
There was a problem hiding this comment.
I wonder if we could use something like isRTL: select( 'core/block-editor' ).getSettings().isRTL, here.
We are not setting this property, but maybe we could overwrite it?
I did a test overwriting settings.isRTL on BlockEditorProvider when it mounts and it works. Not sure if it's the best place to do this though.
There was a problem hiding this comment.
I think that would be good approach according to my previous comment here because it is commonly used on web side.
I also saw this check on web side const isRTL = () => document.documentElement.dir === 'rtl';
Do you think it is aligned with the settings.isRTL ?
There was a problem hiding this comment.
Right! I missed that comment, sorry about that!
This isRTL definition seems to be internal of the component components/src/date-time/date.js.
What I think settings.isRTL does is to apply the RTL layout depending on the language setting set to the WordPress Interface, independent of the browser language, so it's what is mostly used in the project. I believe we would want to use it too.
An example of usage is on the Block Mover
What I'm not sure is where should we call setSettings with the correct isRTL value for mobile. Another chance for this is to set it as soon as the stores are configured, together with setupLocale on gutenberg-mobile repo.
Maybe @Tug will have a better idea about this.
There was a problem hiding this comment.
We might also pass it from the native apps as an init setting in the same way it is set on web, but I don't think is really needed. 🤔
I wonder if we could inject this isRTL setting on the initial props when they arrive to RN, before the editor is set up. Sounds a bit hacky though 🤔
Ok, you're right I replace only the Icon and the whole button should be flipped. I have applied the fix :) |
| this.props.updateSettings( { | ||
| ...this.props.settings, | ||
| isRTL: I18nManager.isRTL, | ||
| } ); |
There was a problem hiding this comment.
@etoledom according to our conversation on isRTL selector I have manage to add isRTL to settings in PBLockEditorProvider this way.
Using it in blocks is very readable and intuitive. Also we don't need to use different markup on web vs native site - it's cross platform after implement in this way. I have refactor other block to align with this - you can check this out.
However I think we need to confirm if it's good place to do this as you mentioned earlier.
There was a problem hiding this comment.
I believe it's the right approach if we need to update this setting dynamically from the editor itself.
What's missing right now though it the ability to "refresh" the editor when the settings change. If we look at the post editor we can see that the EditorProvider get the settings from the editor.
On the web, those settings are given from the PHP side of the plugin. So similarly we would need to pass it as a props from the parent apps.
Concerning isRTL, I guess we can use I18nManager.isRTL instead for now. What do you think about simply overwriting this setting in the native post editor:
At https://github.com/WordPress/gutenberg/blob/master/packages/edit-post/src/editor.native.js#L47
have:
settings = {
...settings,
isRTL: I18nManager.isRTL,
hasFixedToolbar,
focusMode,
};
There was a problem hiding this comment.
Thanks for checking this. I think it make sense. Let me provide changes and well see how it works
|
Thank you @jbinda for the update! Undo-redo looks perfect ✨ New changes looks pretty good too, congrats 🎉
@iamthomasbishop what do you think about it? Any idea when will we have the new proper icon for this? Thank you! I noticed another issue, but I think this one is to be tackled separately since it's not related to RTL in particular (might be related to Aztec).
The Quote block doesn't respect changes on alignment. This also happens on normal LTR layout. (We might have broken it at some point). cc @SergioEstevao @mchowning |
Created an issue to track this. Thanks for spotting the problem @etoledom ! 🙇 I agree that this issue should not block this PR. |
@etoledom @jbinda I made some custom icons for the RTL scenarios. I haven't had a chance to propose them as part of the core GB icon set, so for now, perhaps we can use the ones I've dropped into the design docs. Does this work? I think using RTL-specific icons is preferred over rotating but if we want to continue rotating certain ones, that's fine for now. |
I will give it a shot and let you know. I prepared rotated versions as new icon with suffix |
|
I have add the icons you shared @iamthomasbishop Now it look like this:
|
|
@jbinda Icons look great in the screenshots above. Aren't the bullets for the list supposed to align to the right side of the canvas for RTL? Maybe a known issue, I haven't had a chance to read all of the history here 😬 |
Working on that. I think it's the only one think left in scope of this PR |
|
@jbinda Perfect! Thank you! |
@jbinda I'm afraid that it needs to be done in Aztec. |
Correct, I tried to implement this before, but one of the challenges was that the UITextView adapts the layout and rendering depending of the text language that is passed to it and/or the device system language. |
|
Yeah, so definitely it shouldn't be in the scope of this PR :) |
|
I have excluded 2 below task mentioned in PR description from scope of this project because of above conversation. It will be addressed separately. EXCLUDED tasks:
|
This reverts commit 5e98e2b.
Description
This PR adds missing RTL support for buttons in
GalleryImageand redo/undo buttons inHeaderToolbaron mobile side.TODOS:
GalleryImagemovers buttons in RTL modeHeaderToolbarredo/undobuttons in RTL modeEXCLUDED (according to this comment):
Please also refer to:
Related gutenberg-mobile PR
Related gutenberg-mobile issue
How has this been tested?
To verify point 2 and 3 please follow screenshots provided in related issue description
Screenshots
LTR vs RTL mode
Gallery
MediaText
Paragraph default alignment icon
List icons
Types of changes
Change - add RTL support for below components:
GalleryImagemovers buttonsHeaderToolbarredo/undobuttonsChecklist: