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

Added Filipino, Japanese, Lithuanian, Farsi and Arabic translation #75

Merged
merged 19 commits into from
Apr 30, 2020

Conversation

pjanaya
Copy link
Contributor

@pjanaya pjanaya commented Apr 21, 2020

This PR includes:

New Language Screen:

Japanese:

Filipino

Lithuanian

Arabic

Farsi

@pjanaya pjanaya changed the title Added Filipino and Japanese translation Added Filipino, Japanese, Lithuanian, Farsi and Arabic translation Apr 24, 2020
@pjanaya
Copy link
Contributor Author

pjanaya commented Apr 24, 2020

Update:

  • Added Lithuanian, Farsi, and Arabic to this PR.
  • Updated PR description with screenshots.

Copy link
Member

@eirikurn eirikurn left a comment

Choose a reason for hiding this comment

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

WOW. This is really impressive. Great job overseeing these contributions. Especially the RTL support.

I've never had to deal with that before. Did you check if React Native provides any RTL hooks? Or are we supposed to implement it manually with text-align and everything?

Otherwise it looks pretty good to me. The only thing that stood out in the screenshots is the phone number screen. The checkbox should probably be on the right, along with right aligned text. Also, can you get feedback from the translator about the phone number fields, if the inputs should be switched and right-aligned?

I've sent the new language screen design to the design review. It's probably better in the future to keep those kinds of changes separate.

@eirikurn
Copy link
Member

I discussed this change with the designer. It feels a bit weird that the welcome text is at the bottom of the screen.

This might be a bigger change, but it would probably look better if the welcome text is inside the orange header like on other screens, but fixed (always visible), and the button stays fixed on the bottom. Then the language list can scroll beneath both.

Though we would need to cap the text zooming so it doesn't fill smaller screens.

@pjanaya
Copy link
Contributor Author

pjanaya commented Apr 27, 2020

First off, thanks for the comments and the feedback @eirikurn. I appreciate it a lot :)

Let me discuss a couple things:

  • RTL support: This is definitely a big deal. A little bit of background first. The default layout React Native uses is built with Flexbox. This means we can use the direction: rtl attribute to do this almost auto-magically. One of the things this approach gives is proper transition direction when changing the screen (new pages come to the screen from the left side). The main issue is that you can only enable the full RTL support on load (or reload) of the app. I've searched all over, and it seems to be three ways to do this:
    • Manual changes: This is the approach I took here. It's pretty simple, but the result is not great. You have to make changes to several places, but it will never be perfect. The transition animation won't be correct. However, I guess it's good enough.
    • Using Redux: They talk about this a lot. Keeping the state of the RTL mode using Redux or similar and applying the changes without restarting. I haven't tried so I don't even know if the result is correct (transitions and everything).
    • Reload: Apparently this is the approach with better results, but it has the big downside that you need to reload the app. It's not a super big deal, though. I have a working prototype already. It only reloads if it's needed (LTR->RTL or vice-versa). A couple of seconds, and done. Everything is automatically right-aligned, even the checkbox for the TOS. See the prototype here. The only thing we would need to change is the changeLanguage function:
export const changeLanguage = lang => {
  const restartNeeded = isChangingTextDirection(i18next.language, lang);
  i18next.changeLanguage(lang);
  if (restartNeeded) {
    ReactNative.I18nManager.forceRTL(isRTL());
    RNRestart.Restart();
  }
};
  • Language Screen UI: My original idea was to do what you are suggesting. I even did a quick test before dropping the idea. My problem were two: it required big changes to the <AppShell> element, and the available space for the actual languages was going to be too small in certain screens. I understand the concern about the "Welcome" text being on the bottom, however, I thought it was not a big deal because the actual Welcome screen is the one it comes after

Please, let me know the final decision for both, and I'll try to implement it as soon as I can.

@eirikurn
Copy link
Member

eirikurn commented Apr 27, 2020

Interesting to hear how RTL works. I wonder if the I18nManager defaults to the RTL setting of the OS locale. Then if we default the app locale to the OS locale, it might not need a hard reload in most cases.

But I think it's fine to keep this manual, especially if the issues are fairly minor.

For the language screen UI, we would like to make this screen more consistent with other screens with an orange header. Feel free to revert the UI changes in this PR and continue with it later. Or we can take over it later when we have time.

Thank you so much for your contributions. I really appreciate it.

@pjanaya
Copy link
Contributor Author

pjanaya commented Apr 29, 2020

Ok, I've reverted the changes to the Language Screen, and extended the RTL support to include Text Inputs and the ToS checkbox.

Language Screen (back to normal):

RTL for Text Inputs and checkbox:

Copy link
Member

@eirikurn eirikurn left a comment

Choose a reason for hiding this comment

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

Excellent! Thanks!

@eirikurn eirikurn merged commit 42d169d into aranja:master Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants