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

Breakages, regressions, and new issues in RTL layout #2133

Closed
etoledom opened this issue Apr 8, 2020 · 14 comments · Fixed by #2144
Closed

Breakages, regressions, and new issues in RTL layout #2133

etoledom opened this issue Apr 8, 2020 · 14 comments · Fixed by #2144
Assignees
Labels
RTL [Type] Bug Something isn't working [Type] Regression Existing functionality that got broken

Comments

@etoledom
Copy link
Contributor

etoledom commented Apr 8, 2020

Describe the bug
There have been many new issues in RTL layout.
Related old issue: #1461

Placeholders are gone

This one might be iOS only.

placeholders

  • All RichText placeholders seem to be missing.

Toolbar buttons and List Block

buttons-lists

  • Default text alignment icon is inconsistent with default RTL language alignment
  • Undo and Redo should have flipped position.
    • It's still correct to have Undo pointing to the left, and Redo to the right, since it's a representation of time.
  • List block has the bullets in the opposite side.
  • List block icons need to be flipped (we might need a new ordered list icon if it doesn't exist) cc @iamthomasbishop
    • Keeping "ident" at the left and "unindent" at the right.

Galery block and Inner blocks

gallery-inner

  • Gallery: Image mover icons are reversed.
  • Inner block floating toolbar: Arrows icons need to be flipped.

To Reproduce
Steps to reproduce the behavior:

  • On iOS, the best way is to change the device to a RTL language to have the full experience.
  • Alternatively you can set forceRTL to true.
    • After doing this, you will need to load Gutenberg two times. (Once to set the setting in native side, other for the changes to take place).
      I18nManager.forceRTL( false ); // Change to `true` to debug RTL layout easily.
  • One more option in Xcode is to set Right to left Pseudo-language in language settings in: Product > Scheme > Edit Scheme > Run > Options > Application Language.
@etoledom etoledom added [Type] Bug Something isn't working RTL labels Apr 8, 2020
@etoledom etoledom changed the title Many breakages and new issues in RTL layout Breakages and new issues in RTL layout Apr 8, 2020
@etoledom etoledom added the [Type] Regression Existing functionality that got broken label Apr 8, 2020
@etoledom etoledom changed the title Breakages and new issues in RTL layout Breakages, regressions, and new issues in RTL layout Apr 8, 2020
@iamthomasbishop
Copy link
Contributor

Thank you for surfacing these issues, @etoledom!

Placeholders are gone

😱

Toolbar buttons and List Block

Agreed w/ all of this.

List block icons need to be flipped

We'll need a new G2 icon for this, I will ping the designers on web.

Gallery: Image mover icons are reversed.

@mkevins would you mind taking care of this one?

Inner block floating toolbar: Arrows icons need to be flipped.

@jbinda would you mind taking a look at this, as you're currently working on the floating toolbar? If you're not able to, maybe @chipsnyder could take a look?

@jbinda
Copy link
Contributor

jbinda commented Apr 8, 2020

@iamthomasbishop

No problem I will add RTL support to FloatingToolbar in my PR here

List block icons need to be flipped
We'll need a new G2 icon for this, I will ping the designers on web.
Gallery: Image mover icons are reversed.
@mkevins would you mind taking care of this one?

I have just added a function to ColumnBlock PR which handle rotate the Block movers. I believe that we can also use to flip/switch icons between mentioned buttons in Gallery and List block (undo/redo)

Here is link to my comment about switch. You can check the whole movers file to get the point in wider scope

@jbinda
Copy link
Contributor

jbinda commented Apr 9, 2020

I have implement RTL support for FloatingToolbar:

The changes will be introduce with this PR.

One note here. Does the Save text in right top corned should be reversed in RTL ?

I will also handle rest of breakages in separate PR

@jbinda
Copy link
Contributor

jbinda commented Apr 9, 2020

I have also add support in Gallery Block and undo/redo button. See below screenshots and PR here

LTR vs RTL mode

edit:
missing placeholder in paragraph seems to be RichText issue. If you set text-align right I see it also disappears in LTR mode

@jbinda jbinda mentioned this issue Apr 9, 2020
2 tasks
@iamthomasbishop
Copy link
Contributor

@jbinda looks solid, thank you for fixing that part of this 😄

@jbinda
Copy link
Contributor

jbinda commented Apr 9, 2020

@iamthomasbishop

No problem, there's still some work to do according to my PR TODOs list.

@pinarol
I think maybe it's worth to add RTL mode section to our sanity-testing as well after we fix mentioned issue here ? wdyt ?

@pinarol
Copy link
Contributor

pinarol commented Apr 9, 2020

No harm documenting RTL test cases for sure but Sanity testing is done to make sure the basic & most commonly used functionality is not broken. RTL tests would be a bit too much to run bi-weekly imo. @jbinda

@jbinda
Copy link
Contributor

jbinda commented Apr 10, 2020

I just thought that having this in sanity-test will also point all other places to check if you doing something with RTL. We can also schedule wider timeframe for RTL in sanity-testing schedule (e.g. once per month)

I try to keep sanity-test up to date because it helps me to catch edge case that I might broke in other block during work on some common/shared feature. RTL is one of that kind of feature

Please let me know if something changes.

@jbinda
Copy link
Contributor

jbinda commented Apr 14, 2020

According to List block, shouldn't we also rotate the icon for List block in BottomSheet menu in RTL mode ? See screenshot

@etoledom
Copy link
Contributor Author

According to List block, shouldn't we also rotate the icon for List block in BottomSheet menu in RTL mode ? See screenshot

Good point! I think we should 👍

@jbinda
Copy link
Contributor

jbinda commented Apr 14, 2020

I think I will also check other blocks because I have found issue also in MediaText align in horizontal layout. Icons should be reversed as well because it has rotated orientation according to what layout you see

Anyway I will be update the PR code soon. Stay tuned :)


edited:
I also think that in vertical layout it should be rotate by 90 degrees. Because in vertical layout it changes places up/down instead right/left.

@iamthomasbishop what do you think ?

@jbinda jbinda self-assigned this Apr 14, 2020
@etoledom
Copy link
Contributor Author

Anyway I will be update the PR code soon. Stay tuned :)

Amazing 😁
Good catch!

I also think that in vertical layout it should be rotate by 90 degrees.

Rotating vertically will make the horizontal lines vertical too, right? We might need a new icon.
But maybe it's not necessary since the layout on the final website will still be horizontal. The vertical layout on small screen sizes is more like a special case.

@jbinda
Copy link
Contributor

jbinda commented Apr 14, 2020

Rotating vertically will make the horizontal lines vertical too, right? We might need a new icon.

I think so

But maybe it's not necessary since the layout on the final website will still be horizontal. The vertical layout on small screen sizes is more like a special case.

I think it is kind of UX fix just to be consistent (another example might be the block movers in Columns/Buttons)

Can you take a look once again on PR code ? I dropped some comments

@iamthomasbishop
Copy link
Contributor

According to List block, shouldn't we also rotate the icon for List block in BottomSheet menu in RTL mode ? See screenshot

@jbinda I think that would be good.

I have found issue also in MediaText align in horizontal layout. Icons should be reversed as well because it has rotated orientation according to what layout you see

Good catch. Let's rotate these as well.

I also think that in vertical layout it should be rotate by 90 degrees

I don't think rotating 90% will be a good solution here, we'll need a new icon. Until then let's just use the same mirrored icons as mentioned above. In the long run, this would probably be good, but for now we can just mirror.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RTL [Type] Bug Something isn't working [Type] Regression Existing functionality that got broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants