Update XPF currency formatting #8722
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +608 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
cesarcosta99
requested review from
a team and
rafaelzaleski
and removed request for
a team
cesarcosta99
commented
Apr 30, 2024
Comment on lines
+71
to
+80
| /** | ||
| * Gets wc-admin currency for the given currency code. Unlike `getCurrency`, this function | ||
| * will return the currency based only on the currency itself and locale. This means that the | ||
| * currency object will not be modified based on the store's country settings, | ||
| * or on another given currency. | ||
| * | ||
| * @param {string} currencyCode Currency code | ||
| * @return {Object} Currency object. | ||
| */ | ||
| export const getCurrencyByLocale = ( currencyCode ) => { |
There was a problem hiding this comment.
I created this new helper because I didn't want to risk messing with how getCurrency and formatCurrency work in other parts of the project. The outcome of this function is the same as in getCurrency but it works a little bit differently.
There was a problem hiding this comment.
I believe it was a good trade-off between code repetition and reduced risk of regressions.
Comment on lines
+171
to
+173
| const currency = useLocaleFormatting | ||
| ? getCurrencyByLocale( currencyCode ) | ||
| : getCurrency( currencyCode, baseCurrencyCode ); |
There was a problem hiding this comment.
I didn't want to duplicate logic and, at the same time, wanted it to keep the current behavior in all of its calls. So I just added the useLocaleFormatting param and used it in the currency preview only.
| @@ -930,7 +930,7 @@ | |||
| 'fr_NC' => $global_formats['rs_comma_space_ltr'], | |||
| 'fr_PF' => $global_formats['rs_comma_space_ltr'], | |||
| 'fr_WF' => $global_formats['rs_comma_space_ltr'], | |||
| 'default' => $global_formats['rs_comma_space_ltr'], | |||
| 'default' => $global_formats['ls_dot_comma_ltr'], | |||
There was a problem hiding this comment.
This change should be consistent with woocommerce/woocommerce#46960.
| @@ -2559,7 +2559,7 @@ | |||
| 'currency_code' => 'XPF', | |||
| 'currency_pos' => 'right_space', | |||
| 'thousand_sep' => ' ', | |||
| 'decimal_sep' => ',', | |||
| 'decimal_sep' => '.', | |||
There was a problem hiding this comment.
The changes in this files should be consistent with woocommerce/woocommerce#46960.
Comment on lines
+768
to
+772
| 'defaultLocale' => [ | ||
| 'symbolPosition' => $default_locale_formatting['currency_pos'] ?? '', | ||
| 'thousandSeparator' => $default_locale_formatting['thousand_sep'] ?? '', | ||
| 'decimalSeparator' => $default_locale_formatting['decimal_sep'] ?? '', | ||
| ], |
There was a problem hiding this comment.
I needed to add this new property here because we may need to format a currency from another's country perspective (like in the currency preview). By doing that, the formatting might look odd, that's why I pass on the default locale formatting info.
11 tasks
rafaelzaleski
approved these changes
May 1, 2024
There was a problem hiding this comment.
Good work! The code looks good, and all tests are passing as described.
✅ Test: XPF currency is formatted correctly in the currency preview
✅ Test: XPF currency conversion is working as expected
✅ Test: Another currency (BRL) is formatted correctly in the currency preview
Comment on lines
+71
to
+80
| /** | ||
| * Gets wc-admin currency for the given currency code. Unlike `getCurrency`, this function | ||
| * will return the currency based only on the currency itself and locale. This means that the | ||
| * currency object will not be modified based on the store's country settings, | ||
| * or on another given currency. | ||
| * | ||
| * @param {string} currencyCode Currency code | ||
| * @return {Object} Currency object. | ||
| */ | ||
| export const getCurrencyByLocale = ( currencyCode ) => { |
There was a problem hiding this comment.
I believe it was a good trade-off between code repetition and reduced risk of regressions.
|
Thanks for the review, @rafaelzaleski! I'll merge this as soon as woocommerce/woocommerce#46960 gets merged. |
cesarcosta99
added
the
status: blocked
cesarcosta99
removed
the
status: blocked
hsingyuc
pushed a commit
that referenced
this pull request
May 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #5150.
Changes proposed in this Pull Request
I shared all my findings and decisions on why I'm doing these changes in woocommerce/woocommerce#46960. So here I'll stick to explaining the list of issues that have been flagged and what I did (or didn't) to fix them.
Issue 1
This is no longer an issue, and must have been fixed eventually between the time it's been reported and now. At the moment, the conversion is resulting in the correct price and that can be confirmed by comparing it in any online currency conversion tool.
Issue 2
The symbol is being replaced in WC core. As for the formatting, it's being fixed here, and in WC core, through the changes in the currency and locale infos. These changes should be consistent here and in WC core.
The fix for the formatting here is going beyond updating the currency and locale infos. I explain more about it in the comments below.
Issue 3
The formatting is being fixed here, and in WC core, through the changes in the currency and locale infos. These changes should be consistent here and in WC core.
Testing instructions
Test: XPF currency is formatted correctly in the currency preview
Important
This test replaces the test of same name in woocommerce/woocommerce#46960 as we're applying changes in this PR that are modifying the expected outcome. The difference in the outcome is only relevant from WooPayments' perspective, not WooCommerce's.
npm run build:client.WooCommerce > Settings > Multi-currency.XPFcurrency, if needed.XPF 1,234.Test: XPF currency conversion is working as expected
WooCommerce > Settings > Multi-currency.XPFcurrency, if needed.1for Price rounding and note the CFP franc value.Test: Another currency (BRL) is formatted correctly in the currency preview
WooCommerce > Settings > Multi-currency.BRLcurrency, if needed.R$ 1.234,56. Without these changes, it was previously being formatted likeR$1,234.56, and from the Brazilian perspective, that is not correct.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge