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

[HOLD for payment 2023-03-17] [$4000] Add translations for State & Country Pickers #14549

Closed
Beamanator opened this issue Jan 25, 2023 · 70 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@Beamanator
Copy link
Contributor

Beamanator commented Jan 25, 2023

Problem

In #14290, we created a CountryPicker component that gives the user the option to select many different countries as their home address. This is useful, but if a user prefers to use the app in Spanish (or any other language in the future), we need to make sure to show full address names in that language.

Similarly, the StatePicker we have does not get translated in to Spanish at the moment and it needs to be.

Solution

Update both components to be able to display state / country names in different languages

As agreed upon in this thread, probably the best solution is to put translations in our language files (en.js and es.js) similar to how we have different pronouns lists in those files

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b4c798f34b03ef03
  • Upwork Job ID: 1623242073333895168
  • Last Price Increase: 2023-02-22
@Beamanator Beamanator self-assigned this Jan 25, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 25, 2023
@melvin-bot melvin-bot bot added the Overdue label Feb 6, 2023
@Beamanator
Copy link
Contributor Author

Still on hold

@melvin-bot melvin-bot bot removed the Overdue label Feb 6, 2023
@Beamanator Beamanator changed the title [HOLD #14290] [N7] Add translations for Country Picker (still only store english translation) Add translations for Country Picker (still only store english translation) Feb 7, 2023
@Beamanator
Copy link
Contributor Author

Not on hold anymore! Asked for quick feedback on the plan here: https://expensify.slack.com/archives/C01GTK53T8Q/p1675770086903069

(Will probably also try to translate state picker items in this issue too)

@Beamanator Beamanator changed the title Add translations for Country Picker (still only store english translation) Add translations for State & Country Pickers Feb 8, 2023
@Beamanator Beamanator added the External Added to denote the issue can be worked on by a contributor label Feb 8, 2023
@melvin-bot melvin-bot bot unlocked this conversation Feb 8, 2023
@melvin-bot melvin-bot bot changed the title Add translations for State & Country Pickers [$1000] Add translations for State & Country Pickers Feb 8, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~01b4c798f34b03ef03

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 8, 2023
@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 8, 2023
@MelvinBot
Copy link

Current assignee @Beamanator is eligible for the External assigner, not assigning anyone new.

@Prince-Mendiratta
Copy link
Contributor

Prince-Mendiratta commented Feb 8, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

As of now, all the countries and states displayed using CountryPicker and StatePicker are shown the respective names in only English. We want to support the ability to display the names for the countries and states based on the locale language of the user.

What is the root cause of that problem?

For CountryPicker, we have defined the countries as a constant array in ALL_COUNTRIES object in src/CONST.js.

For StatePicker, the states are listed in an array in english language in the CONST.jsx file in expensify-common repository.

What changes do you think we should make in order to solve the problem?

To tackle this problem for StatePicker, we could shift the States array from expensify-common to the Expensify/App repo and the respective locale files. I do not understand why it was originally kept in a separate library.
If we want to keep the states in the other repository, we could specify the locale specific properties in that object itself. Another thing to note is that right now, we only use the ISO codes for the states. They are language independent. However, we might still want to have localised state name in case we want to add new feature regarding it. Something I have in mind is that if we hover above a specific state code, we can get a hint on the full name of the state.

I'll get the PR ready ASAP as soon as I am assigned to the issue. If the required data about the translation to spanish is not available, I can prepare it myself and then have it reviewed in the PR.

@puneetlath
Copy link
Contributor

@Santhosh-Sellavel thoughts on the proposal?

@melvin-bot melvin-bot bot removed the Overdue label Feb 10, 2023
@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Feb 10, 2023

Thanks for the bump, sound's good at a glance, but there are a few doubts.

Questions to @puneetlath or @Beamanator

  1. Are we fine with this, are we using states anywhere else?

we could shift the States array from expensify-common to the Expensify/App repo and the respective locale files. I do not understand why it was originally kept in a separate library.

  1. Why do we define only states in the e-common repo shouldn't we define the Countries List also?

@Prince-Mendiratta

  1. Shouldn't we use the country code also for countries as we do for states?
  2. Can you share implementation detail in a high-level or pseudo code?
  3. We are not only picking countries via picker, we also allow searching using google to autocomplete in that case suggestions may still show country and state names in English. How are we planning to address that?

@Prince-Mendiratta
Copy link
Contributor

Prince-Mendiratta commented Feb 11, 2023

are we using states anywhere else?

The only other place where we are using the STATES from the lib is in AddressPage to check if the selected state is in USA. We can easily use the locale specific method to use here instead.

  1. Shouldn't we use the country code also for countries as we do for states?

I'm not sure if I understood this completely, country codes are language independent standard and we are currently using country code for phone numbers.

  1. We are not only picking countries via picker, we also allow searching using google to autocomplete in that case suggestions may still show country and state names in English. How are we planning to address that?

We are already doing this, in the AddressSearch, we pass the language: props.preferredLocale to the component and the Google places API automatically returns the results in the locale specific format. More details here.

To test it out, I tried the different locales and here are the results -

Language ScreenShot
English rn_image_picker_lib_temp_e4de00ec-d218-4f16-ae14-e11da460ba2a
Spanish rn_image_picker_lib_temp_f8ef961f-4bb0-4617-9a32-69cff7a1a46f

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Mar 10, 2023
@melvin-bot melvin-bot bot changed the title [$4000] Add translations for State & Country Pickers [HOLD for payment 2023-03-17] [$4000] Add translations for State & Country Pickers Mar 10, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 10, 2023
@MelvinBot
Copy link

Reviewing label has been removed, please complete the "BugZero Checklist".

@MelvinBot
Copy link

MelvinBot commented Mar 10, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.81-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-03-17. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@Prince-Mendiratta
Copy link
Contributor

@Beamanator @Santhosh-Sellavel @0xmiroslav I was working on this issue and it came to my notice that the list of countries is sorted alphabetically as per the country ISO code, instead of country name alphabetically. Do we want to show the list of countries alphabetically according to country name? If so, I'll make those changes when sending the PR for #14958

@Beamanator
Copy link
Contributor Author

Ooh yeah great question @Prince-Mendiratta - I think we would prefer showing alphabetically by country name instead of ISO code, great catch!

@situchan
Copy link
Contributor

I think we would prefer showing alphabetically by country name instead of ISO code

Issue reported today: #16060

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 16, 2023
@puneetlath
Copy link
Contributor

@Prince-Mendiratta @Santhosh-Sellavel sent you hiring offers.

@Prince-Mendiratta
Copy link
Contributor

@puneetlath Thank you, accepted!

@puneetlath
Copy link
Contributor

All paid. Thanks everyone!

@ashimsharma10
Copy link

ashimsharma10 commented Mar 18, 2023

@puneetlath Im the external reporter . And Haven't got paid. I'm new and do not know the procedure

@MelvinBot
Copy link

📣 @ashimsharma10! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@ashimsharma10
Copy link

Contributor details
Expensify account email: 073bct508.ashim@pcampus.edu.np
Upwork Profile Link: upwork.com/freelancers/~018a92cf13e1e88eed

@puneetlath
Copy link
Contributor

@ashimsharma10 can you link me to the slack thread where you reported the issue?

@ashimsharma10
Copy link

ashimsharma10 commented Mar 21, 2023

@puneetlath Im a reporter from #15014 . Issue got solved by this current thread.

@puneetlath
Copy link
Contributor

Ok got it @ashimsharma10. Can you please apply to the Upwork job: https://www.upwork.com/jobs/~01b4c798f34b03ef03

@ashimsharma10
Copy link

@puneetlath Applied .

@gadhiyamanan
Copy link
Contributor

@puneetlath i think this is not eligible for reporting bonus as per #15014 (comment)

@puneetlath
Copy link
Contributor

My takeaway from this comment from @Beamanator is that it should be eligible: #15014 (comment)

@Beamanator what do you think?

@Beamanator
Copy link
Contributor Author

@puneetlath I also don't think this is eligible for reporting bonus, because I believe the fix for that other issue was pretty automatic when implementing the fix for this issue

@ashimsharma10
Copy link

@Beamanator I guess there is no concrete rule on whether to pay or not. But, from a reporter's shoes, it doesn't matter if the issue gets fixed by the same thread or a different one. Anyways, I'm happy that the bug got resolved.

@puneetlath
Copy link
Contributor

Ok sorry for all the confusion on this one. Thanks for being a part of the community @ashimsharma10! We look forward to continuing to work with you 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

8 participants