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

[feature suggestion] Multi-language improvements #3770

Closed
dklymenk opened this issue Jun 27, 2021 · 50 comments
Closed

[feature suggestion] Multi-language improvements #3770

dklymenk opened this issue Jun 27, 2021 · 50 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@dklymenk
Copy link
Contributor

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


While implementing translations on password form screen I noticed that there is currently no way to even see initial screens in Spanish without changing the DEFAULT_LOCALE in CONST.js or setting preferredLocale in Local Storage manually.

I have 2 suggestions:

  1. When user logs out we don't reset his language preference. The implementations for this will look similar to how activeClients are persisted through logout:
diff --git a/src/libs/actions/SignInRedirect.js b/src/libs/actions/SignInRedirect.js
index 9e7b787af..f3d27a335 100644
--- a/src/libs/actions/SignInRedirect.js
+++ b/src/libs/actions/SignInRedirect.js
@@ -19,6 +19,13 @@ Onyx.connect({
     },
 });

+let currentPreferredLocale;
+Onyx.connect({
+    key: ONYXKEYS.PREFERRED_LOCALE,
+    callback: val => currentPreferredLocale = val,
+});
+
+
 /**
  * Clears the Onyx store and redirects to the sign in page.
  * Normally this method would live in Session.js, but that would cause a circular dependency with Network.js.
@@ -36,12 +43,16 @@ function redirectToSignIn(errorMessage) {
     }

     const activeClients = currentActiveClients;
+    const preferredLocale = currentPreferredLocale;

     // We must set the authToken to null so we can navigate to "signin" it's not possible to navigate to the route as
     // it only exists when the authToken is null.
     Onyx.set(ONYXKEYS.SESSION, {authToken: null})
         .then(() => {
             Onyx.clear().then(() => {
+                if (preferredLocale) {
+                    Onyx.set(ONYXKEYS.PREFERRED_LOCALE, preferredLocale);
+                }
                 if (errorMessage) {
                     Onyx.set(ONYXKEYS.SESSION, {error: errorMessage});
                 }
  1. Add a language picker component to a footer. It is already implemented in PreferencesPage, so all we have to do is
    move the original implementation to a new separate component and reuse it in SignInPageLayoutWide and SignInPageLayouyNarrow components. It's going to function like this (I will obviously adjust the styles the whatever would be preferred by your design team):
new-issue-12.mp4

Platform:

Where is this issue occurring?

Web ✔️
iOS ✔️
Android ✔️
Desktop App ✔️
Mobile Web ✔️

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

@dklymenk dklymenk added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 27, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Christinadobrzyn (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 27, 2021
@Christinadobrzyn
Copy link
Contributor

We've had lots of work done to implement E.cash internationalization / localization in Expensify.Cash. So I'm going to add an engineer to this to have them review this as a contributor project.

@MelvinBot
Copy link

Triggered auto assignment to @joelbettner (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Christinadobrzyn Christinadobrzyn removed their assignment Jul 1, 2021
@dklymenk
Copy link
Contributor Author

Hello, are there any updates?

@Christinadobrzyn
Copy link
Contributor

Thanks for following up @dklymenk! No updates yet. We'll add a comment here once there is an update from the team. Thanks!

@joelbettner
Copy link
Contributor

I'm not familiar with what we are doing in terms of internationalization. Perhaps @iwiznia can shed some light on it and give insight into how this proposed change might fit in.

@iwiznia
Copy link
Contributor

iwiznia commented Jul 14, 2021

We are not fully supporting multi languages for now (it's just an internal beta), but this is a small change that makes that beta better, so I'd say we go ahead and do this. Just make sure the selector is only shown when in the beta.

@dklymenk
Copy link
Contributor Author

Hmm, the language picker in preferences seems to be rendered unconditionally: https://github.com/Expensify/Expensify.cash/blob/main/src/pages/settings/PreferencesPage.js#L115-L125

So there doesn't seem to be any beta associated with this feature. If that's a mistake, I guess I can also hide that select if user doesn't have some specific beta. I don't see anything containing locale or lang in my localstorage under betas key, so I would need to ask you for exact beta name to check for.

Thanks.

@iwiznia
Copy link
Contributor

iwiznia commented Jul 14, 2021

Oh damn, you are right, seems we forgot to hide that under a beta...
Can you add that? Let's name the beta inetrnationalization, add a method to check for it in the Permissions file and call it in both places where we use the selector. Or maybe is better to have the check in the selector itself? What do you think?

@dklymenk
Copy link
Contributor Author

Sure I will add that.

I've taken a look at Permissions.js and I don't a see any reason not to use it. I wouldn't need to add any dev mode checks myself to show the selector in dev mode. Also it would be very confusing if some permissions were checked in components and some in Permissions.js.

Basically, if the requested design changes for the sector on login page will be minimal I'll create a new component for it, that will be reused on login screen and preferences, then inside that component I'll call a new function canUseInternationalization from Permissions.js to know whether to render the selector or not.

Is my first suggestion from original post good too? The one about persisting preferred locale through logout. Before I begin, I'd like to know if I'm implementing that one too.

@iwiznia
Copy link
Contributor

iwiznia commented Jul 14, 2021

Basically, if the requested design changes for the sector on login page will be minimal I'll create a new component for it, that will be reused on login screen and preferences, then inside that component I'll call a new function canUseInternationalization from Permissions.js to know whether to render the selector or not.

Sounds great.

Is my first suggestion from original post good too? The one about persisting preferred locale through logout. Before I begin, I'd like to know if I'm implementing that one too.

Yep, makes total sense to not lose the language you were using when logging out.

@dklymenk
Copy link
Contributor Author

Ok, great.

I will also need to know if the selector on the video looks ok, design-wise. I think there is at least some margin needed between legal text and the selector. Maybe there is more changes required for size, color, padding, position, etc. Please let me know if that's the case.

Otherwise, I think I'm ready to begin, so I'm waiting for an upwork contract.

Thanks.

@iwiznia
Copy link
Contributor

iwiznia commented Jul 14, 2021

For that I'll tag @shawnborton

@iwiznia iwiznia added the External Added to denote the issue can be worked on by a contributor label Jul 14, 2021
@MelvinBot
Copy link

Triggered auto assignment to @kevinksullivan (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@iwiznia
Copy link
Contributor

iwiznia commented Jul 14, 2021

@kevinksullivan can you create a contract in upwork and hire @dklymenk please?

@kevinksullivan
Copy link
Contributor

@dklymenk
Copy link
Contributor Author

@iwiznia PR #4134 is ready for review.

@shawnborton I have not removed text-align: center; from legal text since it looked like a mistake. Please let me know if that's not the case, and I'll update the PR accordingly.

@shawnborton
Copy link
Contributor

shawnborton commented Jul 21, 2021

I actually did make the text-align: left on purpose as I thought it looked better given that the logo was on the left edge and the language selector was on the right edge. The text/elements above in the form are left-aligned as well. What do you think?

@dklymenk
Copy link
Contributor Author

Ah, yeah. Now that I look at it, it makes sense. Thanks for the classification, I'll push the change shortly.

dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jul 21, 2021
dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jul 21, 2021
@dklymenk
Copy link
Contributor Author

Legal text is now aligned the left. PR updated.

Thanks.

dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jul 21, 2021
@iwiznia iwiznia added Weekly KSv2 and removed Daily KSv2 labels Jul 22, 2021
@MelvinBot MelvinBot removed the Overdue label Jul 22, 2021
dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jul 22, 2021
dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jul 22, 2021
dklymenk added a commit to dklymenk/Expensify.cash that referenced this issue Jul 22, 2021
@kevinksullivan
Copy link
Contributor

@dklymenk when you get a chance please submit a proposal to the upwork post so I can hire you.

@dklymenk
Copy link
Contributor Author

@kevinksullivan I have withdrawn my proposal. I thought I could resubmit it, but I can no longer submit it now...

Is it possible for you to invite me directly? https://www.upwork.com/freelancers/~01ab1cc8748e129eaa

@kevinksullivan
Copy link
Contributor

All set @dklymenk ! Hired you directly.

@dklymenk
Copy link
Contributor Author

Yep. Accepted. Thank you!

@iwiznia
Copy link
Contributor

iwiznia commented Aug 4, 2021

This is done, was deployed to prod 5 days ago...

@MelvinBot MelvinBot removed the Overdue label Aug 4, 2021
@kevinksullivan
Copy link
Contributor

merged 8 days ago, and we typically wait 7. Paid.

@dklymenk
Copy link
Contributor Author

dklymenk commented Aug 5, 2021

Hello @kevinksullivan, can you please check again. The contract is still open for me and I haven't received any notifications about payments either.

@aman-atg
Copy link
Contributor

aman-atg commented Aug 5, 2021

@kevinksullivan , I think you have accidentally made payment for #4219 instead of this one.

@dklymenk
Copy link
Contributor Author

dklymenk commented Aug 9, 2021

Hello @kevinksullivan , are there any updates on the contract status?

@kevinksullivan
Copy link
Contributor

So sorry @dklymenk , I must've gotten upwork jobs with similar names confused. Can you confirm you have received payment now?

@dklymenk
Copy link
Contributor Author

Hello, yes. I have recieved a payment, but I think you might have forgotten about the finder's bonus: https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#proposing-a-job-that-expensify-hasnt-posted

@kevinksullivan
Copy link
Contributor

kevinksullivan commented Aug 16, 2021

All set @dklymenk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants