Skip to content

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Sep 17, 2018

In RTL mode, the body element is expected to have a rtl CSS class. This patch ensures that the class is added/removed when switching locales dynamically at runtime.

Some styles, i.e., the ones that horizontally flip Gridicon arrows, depend on the body.rtl class being present.

I discovered this bug when working on building our CSS with Webpack.

How to test:
Go to login page and enable the quick language switcher:

https://hash.calypso.live/log-in?flags=quick-language-switcher

Switch language to Hebrew using the Quick Language Switcher and watch the "Back" arrow at the bottom of the login UI:

login-arrow-rtl

Actual result (wrong): the arrow points to the left
Expected result (correct): the arrow points to the right, i.e., "back" in RTL mode

Verify that the expected result starts happening after this patch is applied.

Careful observer will notice that the link with the "Back" arrow should be aligned to the right edge in RTL mode, but in reality it's aligned to the left. That's another bug where the section stylesheet is not switched from LTR to RTL in certain scenarios, one of the being using the Quick Language Switcher. Will be solved later.

…lass

In RTL mode, the `body` element is expected to have a `rtl` CSS class. This patch
ensures that the class is added/removed when switching locales dynamically at runtime.

Some styles, i.e., the ones that horizontally flip Gridicon arrows, depend on the `body.rtl`
class being present.
@jsnajdr jsnajdr added [Type] Bug When a feature is broken and / or not performing as intended i18n labels Sep 17, 2018
@jsnajdr jsnajdr self-assigned this Sep 17, 2018
@matticbot
Copy link
Contributor

@jsnajdr jsnajdr requested review from Tug, a team, sirreal, ockham, yoavf and akirk September 17, 2018 12:09
@ockham
Copy link
Contributor

ockham commented Sep 17, 2018

Hmm I thought we were already doing that... somewhere? Maybe somone from Team Global can enlighten. /cc @southp @ramonjd

@yoavf
Copy link
Contributor

yoavf commented Sep 17, 2018

@ockham I'm not too familiar with the language switching mechanism, but when I test on https://wpcalypso.wordpress.com/log-in/ and I switch to Hebrew by clicking on the language detector prompt, there's a quick flash of the version missing the RTL tag, then some kind of reload for the correct version.
n4aqz4yl2p

The reload also happens on this branch, but at least the initial version is correct :)

Copy link
Contributor

@yoavf yoavf left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@jsnajdr
Copy link
Member Author

jsnajdr commented Sep 17, 2018

Hmm I thought we were already doing that... somewhere?

There are two elements that have an RTL-ish attribute: html and body:

<html dir="rtl">
  <body class="rtl">
</html>

We already change the html[dir] attribute, but body[class] we don't. That's what this patch fixes.

@jsnajdr jsnajdr merged commit d7daf48 into master Sep 18, 2018
@jsnajdr jsnajdr deleted the fix/locale-switch-body-rtl branch September 18, 2018 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants