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

Replace the existing US-located numbers, date and amounts with locale-aware components/ methods #2349

Closed
jboniface opened this issue Apr 12, 2021 · 23 comments · Fixed by #3021
Assignees

Comments

@jboniface
Copy link

jboniface commented Apr 12, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


We are implementing the structure for internationalisation and localisation in Expensify.Cash.

As part of that, we need to replace the existing US-located numbers, dates, amounts and phone numbers with locale-aware components/ methods. In order to do that, you need to identify all places that display any of that information and then use the methods numberFormat, timestampToRelative, timestampToDateTime, toLocalPhone, fromLocalPhone of the withLocalize HOC we added here.

Expected Result:

numbers, dates, and amounts should be locale-aware

Actual Result:

They are US centric

Platform:

All

Version Number: current
https://github.com/Expensify/Expensify/issues/152154

Job on Upwork

view Upwork job here

@iwiznia
Copy link
Contributor

iwiznia commented Apr 19, 2021

b) then replace them with the HOA or do the necessary small refactorings.

Sorry, what's HOA?

Also, can you provide a list of places you'll update?

@Andmat7
Copy link

Andmat7 commented Apr 19, 2021

A thousand apologies HOC, the corrector corrected me wrong, do I make a list with all the files that are going to be corrected?

@iwiznia
Copy link
Contributor

iwiznia commented Apr 19, 2021

Yeah, you can link to every text/file that will need updating

@iwiznia
Copy link
Contributor

iwiznia commented Apr 19, 2021

Mainly I was asking for the list to make sure we are not missing any place to update and that you are not on the hook for any new place we use numbers/dates/amounts that get added after your PR is over.
If you don't want to create the list now, that's fine, but we will be asking for followup PRs for any places that are missing, before paying. Let me know how you want to proceed.

@iwiznia
Copy link
Contributor

iwiznia commented Apr 19, 2021

That line is not rendering the amount, so that would not be the place to localize it, it would be here

@iwiznia
Copy link
Contributor

iwiznia commented Apr 20, 2021

@Andmat7 I see you deleted several of your comments, any reason why? If you want to work on this, please re-post here your proposal.

@parasharrajat
Copy link
Member

parasharrajat commented May 10, 2021

Here is an outline

  1. We will use withLocalize HOC to use the numberFormat, timestampToRelative, timestampToDateTime, toLocalPhone, fromLocalPhone method.
  2. We replace all the places where we see number and time date with the following method calls
    e.g.
    https://github.com/Expensify/Expensify.cash/blob/9fbf82e8237b7d1ff416619970038cc0d5bf9252/src/pages/iou/steps/IOUAmountPage.js#L107
    to
<Text style={styles.iouAmountText}>{this.props.translations.numberFormat(this.state.amount)}</Text>

Here is the list of most places where we can apply these methods.

  1. https://github.com/Expensify/Expensify.cash/blob/9fbf82e8237b7d1ff416619970038cc0d5bf9252/src/pages/signin/ChangeExpensifyLoginLink.js#L28
  2. https://github.com/Expensify/Expensify.cash/blob/9fbf82e8237b7d1ff416619970038cc0d5bf9252/src/components/IOUBadge.js#L31
  3. https://github.com/Expensify/Expensify.cash/blob/a75a4077b526417c09ed07316334a707cd2feea2/src/pages/home/report/ReportActionItemDate.js#L14
  4. https://github.com/Expensify/Expensify.cash/blob/6fcfb3fea0a0d72ae269226e9e88402607c5fa61/src/components/ReportActionItemIOUPreview.js#L77
  5. When type is phone
    https://github.com/Expensify/Expensify.cash/blob/a75a4077b526417c09ed07316334a707cd2feea2/src/pages/settings/Profile/LoginField.js#L103
  6. https://github.com/Expensify/Expensify.cash/blob/6fcfb3fea0a0d72ae269226e9e88402607c5fa61/src/pages/DetailsPage.js#L85
  7. https://github.com/Expensify/Expensify.cash/blob/6fcfb3fea0a0d72ae269226e9e88402607c5fa61/src/pages/DetailsPage.js#L103
    Updating Shortly after answers to the Questions

Most of the locations expect string or number so we have to conditionally apply the Methods.
for e.g. https://github.com/Expensify/Expensify.cash/blob/6fcfb3fea0a0d72ae269226e9e88402607c5fa61/src/pages/DetailsPage.js#L86

Questions:

@iwiznia

  1. Do we need to replace the phone number and input values as well. For eg: on the login form, we have an input field where the user can type the phone number to log in?
  2. I see that Phone number is also visible at LHN like this. Do we need to added the methods there as well>
    image

@iwiznia
Copy link
Contributor

iwiznia commented May 11, 2021

We replace all the places where we see number and time date with the following method calls

Watch out because the amounts examples are not quite correct. When we are dealing with a money amount, we need to format it as so by passing the currency and style:'currency' to the options of the numberFormat call. You can see some examples here.

Do we need to replace the phone number and input values as well. For eg: on the login form, we have an input field where the user can type the phone number to log in?

Good question, I think that on login we should not do anything, but I think it would be nice if we allowed you to for example start a new chat with a phone number by only typing the number without the international prefix and then calling fromLocalPhone

I see that Phone number is also visible at LHN like this. Do we need to added the methods there as well>

Yes

Proposal looks good, @jboniface can you go ahead and hire @parasharrajat in upwork?

@parasharrajat
Copy link
Member

@iwiznia I checked it when posting the proposal but Actually, we render the Sign separately https://github.com/Expensify/Expensify.cash/blob/9fbf82e8237b7d1ff416619970038cc0d5bf9252/src/pages/iou/steps/IOUAmountPage.js#L104. So we just need to convert the amount. But we can find the best possible things after changes as I think I would be needing some feedback while working over this issue.

@iwiznia
Copy link
Contributor

iwiznia commented May 11, 2021

hmmmmm and can't we transform that to be just one component?

@parasharrajat
Copy link
Member

parasharrajat commented May 11, 2021

Yeah. That's one of the Ideas as for some of the locales currency sign is shown after the amount. Let's look at these changes while reviewing the PR to be exact.

@iwiznia
Copy link
Contributor

iwiznia commented May 11, 2021

Yeah, exactly, Intl.numberFormat (which is what our numberFormat uses) handles those cases and puts the currency after/before the number based on the locale.

@iwiznia
Copy link
Contributor

iwiznia commented May 19, 2021

@parasharrajat what is the status of this?

@parasharrajat
Copy link
Member

@iwiznia Sorry for delay on this. I will add the PR today.

@parasharrajat
Copy link
Member

@iwiznia Question:

  1. We have IOUBadge which currently shows the requested amount but it has a fixed $ Sign. What should we do here? should take the currency sign from numberFormat method, but for that, we need a currency parameter.
    https://github.com/Expensify/Expensify.cash/blob/bbd386709f6d3a25d4b62f7889a3213b6ee45e12/src/components/IOUBadge.js#L31

@iwiznia
Copy link
Contributor

iwiznia commented May 19, 2021

hmmmm the currency should be in the report stored in Onyx, so I guess you can use props.iouReport.currency, no?

@parasharrajat
Copy link
Member

Not sure what is the data structure for that and how that works so it's hard to imagine. If you can lead me to any docs on IOU, I can better understand the amount used on different components under different contexts.

let me check this. Thanks.

@iwiznia
Copy link
Contributor

iwiznia commented May 19, 2021

All reports have the same data structure, which is generated by getSimplifiedReportObject. The IOUs in particular are set here and here

@parasharrajat
Copy link
Member

parasharrajat commented May 20, 2021

@iwiznia Here on LHN. Do I need to remove the country code from the
Screenshot 2021-05-21 02:05:13
phone number.

There are a couple of other places where we show phone numbers like this.
In general, do I need to remove the country code from all the places where the phone number is visible?

I saw that for IOUBadge https://github.com/Expensify/Expensify.cash/blob/bbd386709f6d3a25d4b62f7889a3213b6ee45e12/src/components/IOUBadge.js#L31

Num.numberformat is important as we receive the amount of 1000 for $10 from the backend and it will give us a string like $10.00.We can't pass that string to localized numberFormat How should I handle this amount to use in localized NumberFormat.

@iwiznia
Copy link
Contributor

iwiznia commented May 20, 2021

There are a couple of other places where we show phone numbers like this.
In general, do I need to remove the country code from all the places where the phone number is visible?

Yep

Num.numberformat is important as we receive the amount of 1000 for $10 from the backend and it will give us a string like $10.00.We can't pass that string to localized numberFormat How should I handle this amount to use in localized NumberFormat.

You need to use the locale aware method. If you are using the withLocalize HOC, then it would be: this.props.numberFormat(props.iouReport.total / 100, {type: 'currency', currency: props.iouReport.currency})

@parasharrajat
Copy link
Member

Questions:

  1. In the below image, the country code of the phone number is of the user whose number is shown there? but if we want to remove the +{countryCode}, our localized toLocalPhone will not work as it works only for the user who is logged in. WithLocalize usages the locale of the current user. And thus I believe we don't want to remove the country code from these places otherwise logged-in users won't be able to know what is the other user's exact country? Does it make sense?

https://user-images.githubusercontent.com/24370807/119046053-dfe06900-b9d9-11eb-8f9e-52d483bf62b5.png

This makes me wonder that there are very few places we need to convert numbers and amounts. And we need to revise the list once more.

@iwiznia
Copy link
Contributor

iwiznia commented May 24, 2021

The idea behind the toLocalPhone is to not see the country code of phones in your location. So if I am in USA I would not see all US phones prefixed with +1 but I would see all the non US phones with their prefix. Or in other words, if a phone does not have a prefix, it's because it's in your own country.

@parasharrajat
Copy link
Member

Gotcha. Adding now. Then I will add you on the PR to review.

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 a pull request may close this issue.

4 participants