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.

Sign up for GitHub

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

Jump to bottom

RTL layout fixes #585

Merged
merged 10 commits into from
Feb 14, 2019
Merged

RTL layout fixes #585

merged 10 commits into from
Feb 14, 2019

Conversation

etoledom
Copy link
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
@@ -8,6 +8,8 @@ const gutenbergSetup = () => {
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
Contributor Author

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
Contributor

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
Contributor Author

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

@daniloercoli
Copy link
Contributor

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

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

@daniloercoli
Copy link
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
Contributor Author

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
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
Contributor Author

Thank you!

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

@daniloercoli daniloercoli daniloercoli approved these changes

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