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

fix(l10n): fixed currency localization for "other than US" locales #5948

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

dannycoates
Copy link
Contributor

For en locales that don't have their own ftl file this change
sets their FluentResource to be that of en-GB, which does have
an ftl file in order for the currency to be rendered as 'US$x.xx'
instead of the US fallback of '$x.xx' which is ambiguous in which
currency is meant.

These locales are currently hard coded in AppLocalizationProvider
because other services fail when they can't load l10n files that
are present in supportedLanguages.json but not in the l10n repo.

For en locales that don't have their own ftl file this change
sets their FluentResource to be that of en-GB, which does have
an ftl file in order for the currency to be rendered as 'US$x.xx'
instead of the US fallback of '$x.xx' which is ambiguous in which
currency is meant.

These locales are currently hard coded in AppLocalizationProvider
because other services fail when they can't load l10n files that
are present in supportedLanguages.json but not in the l10n repo.
Copy link
Contributor

@chenba chenba left a comment

Choose a reason for hiding this comment

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

Appears to be the indirection we want!

@dannycoates
Copy link
Contributor Author

I tried adding a test to AppLocalizationProvider.test.js but it ends up with no US specifier, i.e. $1.23. It works in a real browser so I wonder if this is a quirk of the test harness? @lmorchard @chenba ideas?

@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #5948 into main will decrease coverage by 0.03%.
The diff coverage is 96.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5948      +/-   ##
==========================================
- Coverage   93.84%   93.81%   -0.04%     
==========================================
  Files         299       58     -241     
  Lines       14417     1551   -12866     
  Branches      385      412      +27     
==========================================
- Hits        13530     1455   -12075     
+ Misses        884       95     -789     
+ Partials        3        1       -2     
Impacted Files Coverage Δ
...yments-server/src/components/PaymentForm/index.tsx 100.00% <ø> (+1.35%) ⬆️
packages/fxa-payments-server/src/index.tsx 14.03% <ø> (ø)
...ackages/fxa-payments-server/src/lib/AppContext.tsx 80.00% <ø> (ø)
packages/fxa-payments-server/src/lib/apiClient.ts 92.85% <0.00%> (-7.15%) ⬇️
...outes/Subscriptions/Reactivate/ManagementPanel.tsx 100.00% <ø> (ø)
...rver/src/routes/Subscriptions/SubscriptionItem.tsx 90.47% <ø> (-0.44%) ⬇️
packages/fxa-payments-server/src/store/state.ts 100.00% <ø> (ø)
packages/fxa-payments-server/src/App.tsx 65.51% <50.00%> (-1.15%) ⬇️
...erver/src/components/PaymentConfirmation/index.tsx 100.00% <100.00%> (ø)
...ents-server/src/components/PaymentFormV2/index.tsx 100.00% <100.00%> (ø)
... and 242 more

@chenba
Copy link
Contributor

chenba commented Jul 14, 2020

I tried adding a test to AppLocalizationProvider.test.js but it ends up with no US specifier, i.e. $1.23. It works in a real browser so I wonder if this is a quirk of the test harness? @lmorchard @chenba ideas?

Is there a gist or branch I can look at?

@dannycoates
Copy link
Contributor Author

@chenba
Copy link
Contributor

chenba commented Jul 14, 2020

Yeah, getLocalizedCurrency still has en-US hard coded for its locale. 😭

@chenba
Copy link
Contributor

chenba commented Jul 14, 2020

In other words, your test is too good. I think we just need to know a string from 'en-GB' is used when the locale is 'en-NZ'?

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.

None yet

3 participants