Skip to content

RTL layout fixes#585

Merged
etoledom merged 10 commits intodevelopfrom
issue/rtl-fixes
Feb 14, 2019
Merged

RTL layout fixes#585
etoledom merged 10 commits intodevelopfrom
issue/rtl-fixes

Conversation

@etoledom
Copy link
Copy Markdown
Contributor

@etoledom etoledom commented Feb 11, 2019 •
edited
Loading

Fixes #380

This PR fixes two issues related to RTL layout.

rtl

1. The block Toolbar on Android:

The content of the scroll view on Android was aligned left, while on iOS it was correctly aligned right. Probably the issue has little to do with alignment itself but on how each platform rotate scroll view for RTL support.

This was fix by forcing the content to grow as big as the scroll view itself (allowing it to grow bigger if necessary).

2. BottomSheet text input:

Fixed in the gutenberg related PR (WordPress/gutenberg#13815)

This was an issue since we are inverting the natural alignment for text inputs, so we had to express this intention on RTL layout too. Sadly text-align: end didn't work (it's not allowed).

I added a helper to test RTL layout, but I'm unsure if we should keep for continue testing RTL in the future. Ideally we would have a switch in the UI or use the RN Dev Menu, but this last option requires native development.

To test:

  • Run the project.
  • Go to src/index.js and switch I18nManager.forceRTL(false); to true.
  • Reload the app (you might need to reload more than once for some reason)
    • Now the layout should look mirrored horizontally (RTL)
    • Note that elements like images and icons are not inverted 👍
  • Check that the ToolBar displays correctly.
  • Edit a Header block and check that the content of the ToolBar is still scrollable.
  • Display the Image Settings BottomSheet. (Wheel icon on a non-empty image block)
  • Check that the TextInputs are displayed properly.

@etoledom etoledom added [Type] Enhancement Improves a current area of the editor RTL labels Feb 11, 2019
@etoledom etoledom added this to the Beta milestone Feb 11, 2019
@etoledom etoledom self-assigned this Feb 11, 2019
@etoledom etoledom mentioned this pull request Feb 11, 2019
Merged
etoledom
etoledom commented Feb 11, 2019
View reviewed changes
src/index.js
const apiFetch = require( '@wordpress/api-fetch' ).default;
const wpData = require( '@wordpress/data' );

I18nManager.forceRTL( false ); // Change to `true` to debug RTL layout easily.
Copy link
Copy Markdown
Contributor Author

@etoledom etoledom Feb 11, 2019

Choose a reason for hiding this comment

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

Not sure if we should leave this here. But for the moment it could be useful for fast checking RTL layout in both iOS and Android, without the need of AndroidStudio or Xcode or changing settings on device.

Copy link
Copy Markdown
Contributor

@daniloercoli daniloercoli Feb 12, 2019

Choose a reason for hiding this comment

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

I guess we can leave it here. It doesn't hurt, no?

@etoledom
Copy link
Copy Markdown
Contributor Author

etoledom commented Feb 11, 2019

Hey @daniloercoli - Can I bug you with a review?
Thank you! :)

@daniloercoli
Copy link
Copy Markdown
Contributor

daniloercoli commented Feb 12, 2019

  • Note that elements like images and icons are not inverted 👍

It seems that H* are inverted instead. Is it correct?

@daniloercoli
Copy link
Copy Markdown
Contributor

daniloercoli commented Feb 12, 2019 •
edited
Loading

Tested this PR on both iOS and Android, and it seems to work fine. LGTM once gb conflicts are resolved.

Note related to this PR, but i've noticed that the Image Settings BottomSheet is not dismissed when I tap back button on Android. I need to tap outside the sheet to dismiss it. (Tested on Android 6 - Nexus 5).

@etoledom
Copy link
Copy Markdown
Contributor Author

etoledom commented Feb 12, 2019

Thank you @daniloercoli for the review!

i've noticed that the Image Settings BottomSheet is not dismissed when I tap back button on Android

Good catch! If I recall correctly, @marecar3 also noticed it and was working on a fix. I'm not really sure if that's the case. If it's not, I can check that out. 👍

@koke koke mentioned this pull request Feb 13, 2019
Closed
4 tasks
@hypest hypest mentioned this pull request Feb 13, 2019
Closed
daniloercoli
daniloercoli approved these changes Feb 14, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@daniloercoli daniloercoli left a comment

Choose a reason for hiding this comment

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

LGTM!

@etoledom etoledom merged commit 6d9a3fc into develop Feb 14, 2019
@etoledom etoledom deleted the issue/rtl-fixes branch February 14, 2019 12:35
@etoledom
Copy link
Copy Markdown
Contributor Author

etoledom commented Feb 14, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@daniloercoli daniloercoli daniloercoli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@etoledom etoledom

Labels

RTL [Type] Enhancement Improves a current area of the editor

Projects

None yet

Milestone

Beta

Development

Successfully merging this pull request may close these issues.

Make sure RTL text is handled correctly

2 participants

@etoledom @daniloercoli