Added Localize HOC #2208

parasharrajat · 2021-04-01T23:31:43Z

Please review.

Details

Created a HOC named WithLocalize which set the preferredLocale to a list of functions and passes curried methods down to WrappedComponent.
Created one method numberFormat which uses default Browser Intl to convert the number.
Added polyfills for Intl.numberFormat for mobile platforms.

Fixed Issues

Fixes #2176

Tests / QA Steps

Wrap any component with newly created HOC and use the method as props.translations.method(params). Switch the preferred locale for the user(I am not sure How to change the Locale in App) and see this method call should give you output based on the new Locale.

Tested On

I just tested it using console output and manually updating the locale on Onyx.

src/components/withLocalize.js

iwiznia · 2021-04-06T20:16:23Z

src/libs/numberFormat/index.native.js

+// Load en Locale data
+import '@formatjs/intl-numberformat/locale-data/en';
+
+function numberFormat(locale, number, options) {


Also, is there a way to not have to define this twice? And if there is, do we think is worth it?

We just need to import those files for mobile(native) platforms. I think it can be done via webpack. But for locale #2208 (review)

I understand but the numberFormat is defined duplicate in 2 places.
(also, that link is supposed to send me where exactly? It takes me here again)

Yeah. So I am saying that we can load the imports via webpack and remove two files.

And for locale probably

A proper way would be when the user switches a locale, load the file remotely.

Yeah. So I am saying that we can load the imports via webpack and remove two files.

I am not sure I know how this works, can you explain?

You can import the src/libs/numberFormat/index.js which already defines the function and use it here, no?

index.js will be replaced with index.native.js so we need a new file. then we import from that file. Or better would be to create a file called lib/polyfills/index.native.js and import the packages there. this will allow adding all the polyfills at a single location and we import this file in app.js.

index.js will be replaced with index.native.js

Huh? Why?

What I am saying is:

The method is in index.js

index.native.js would import it and use it

Ok. let me test it first to be sure. My assumption is that we replace index.js files with index.native.js on mobile platforms.

@iwiznia. I tested it. My assumptions are correct. index files are replaced with platform-specific index files(such as index.android.js on android). So I can make a common base.js file. or let's just leave it like this as this is just two lines of code. We had to create two files.

Index.js

index.android.js
To keep it dry, we can create one more base.js, But I think that will be totally overuse of DRY for just two lines. Anyways each platform only gets one file in the bundle.

src/libs/numberFormat/index.native.js

src/components/withLocalize.js

src/libs/numberFormat/index.native.js

iwiznia · 2021-04-06T20:25:35Z

. Switch the preferred locale for the user(I am not sure How to change the Locale in App) and see this method call should give you output based on the new Locale.

So you did not really test this? Right now there's no UI to change the locale, can you do some tests by changing it via code or something like that?

parasharrajat · 2021-04-06T20:36:36Z

@iwiznia I tested it by loading one other locale file. tried these examples from MDN https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat.

So for simple testing.

Load another locale file. and then call the method with that locale and see the output changes.

parasharrajat · 2021-04-07T18:43:37Z

Updated. Please review.

iwiznia · 2021-04-07T21:17:29Z

src/libs/numberFormat/index.android.js

+function numberFormat(locale, number, options) {
+    return new Intl.NumberFormat(locale, options).format(number);
+}


Can you test if importing and using the function defined in index.js works properly please?

Oh, I missed this comment

iwiznia · 2021-04-07T21:18:58Z

To clarify, did you manage to test on iOS? Seems like you did not?

parasharrajat · 2021-04-07T21:26:39Z

@iwiznia I commented it above.

Ok, I tested it on IOS. You are right. IOS does have Intl support. Check the snap here

you can see the output on IOS via following the link.

OSBotify · 2021-04-07T21:48:20Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

parasharrajat added 3 commits April 2, 2021 04:55

Added Localize HOC

d56a924

new: added numberFormat method

45cdac0

props update

46d4393

parasharrajat marked this pull request as ready for review April 3, 2021 00:33

parasharrajat requested a review from a team as a code owner April 3, 2021 00:33

MelvinBot requested review from pecanoro and removed request for a team April 3, 2021 00:33

parasharrajat mentioned this pull request Apr 6, 2021

Expensify.cash - Implement Higher-Order Component that re-renders when the locale changes #2176

Closed

parasharrajat commented Apr 6, 2021

View reviewed changes

src/components/withLocalize.js Show resolved Hide resolved

iwiznia reviewed Apr 6, 2021

View reviewed changes

parasharrajat commented Apr 6, 2021

View reviewed changes

src/libs/numberFormat/index.native.js Outdated Show resolved Hide resolved

remove numbeformat Polyfills from IOS

965f92f

iwiznia reviewed Apr 7, 2021

View reviewed changes

iwiznia removed the request for review from pecanoro April 7, 2021 21:18

iwiznia approved these changes Apr 7, 2021

View reviewed changes

iwiznia merged commit fbf0864 into Expensify:master Apr 7, 2021

jboniface mentioned this pull request Apr 12, 2021

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

Closed

michaelhaxhiu mentioned this pull request Apr 15, 2021

Apply localization to all hard coded text in the Expensify.cash app #2408

Closed

5 tasks

Prince-Mendiratta mentioned this pull request Jan 24, 2023

[HOLD for payment 2023-03-01] [$4000] Few currency symbols (specially symbols after WST) are not correctly displayed on mWeb chrome on android #14307

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Localize HOC #2208

Added Localize HOC #2208

parasharrajat commented Apr 1, 2021 •

edited

Loading

iwiznia Apr 6, 2021

parasharrajat Apr 6, 2021

iwiznia Apr 6, 2021

parasharrajat Apr 6, 2021

iwiznia Apr 6, 2021

iwiznia Apr 6, 2021

parasharrajat Apr 6, 2021

iwiznia Apr 6, 2021

parasharrajat Apr 6, 2021

parasharrajat Apr 7, 2021 •

edited

Loading

iwiznia commented Apr 6, 2021

parasharrajat commented Apr 6, 2021

parasharrajat commented Apr 7, 2021

iwiznia Apr 7, 2021

iwiznia Apr 7, 2021

iwiznia commented Apr 7, 2021

parasharrajat commented Apr 7, 2021 •

edited

Loading

OSBotify commented Apr 7, 2021

Added Localize HOC #2208

Added Localize HOC #2208

Conversation

parasharrajat commented Apr 1, 2021 • edited Loading

Details

Fixed Issues

Tests / QA Steps

Tested On

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

iwiznia commented Apr 6, 2021

parasharrajat commented Apr 6, 2021

parasharrajat commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwiznia commented Apr 7, 2021

parasharrajat commented Apr 7, 2021 • edited Loading

OSBotify commented Apr 7, 2021

parasharrajat commented Apr 1, 2021 •

edited

Loading

parasharrajat Apr 7, 2021 •

edited

Loading

parasharrajat commented Apr 7, 2021 •

edited

Loading