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
Update symbol and formatting of the XPF currency #46960
Conversation
@rafaelzaleski, requesting your review here as this is tied to Automattic/woocommerce-payments#8722. |
Hi @rafaelzaleski, @vedanshujain, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good and all tests have passed as described.
However, I've noticed some failed checks in the CI. Could you address these issues? For instance, in this file, you'll need to include the changes to the XPF currency for the tests.
This reverts commit 51abcf4.
Sorry I missed that, @rafaelzaleski. I fixed it in 9e5e3e3. The CI was flagging some linting issues after my fix. I tried to fix them in 51abcf4, but ended up reverting it in f531016 because the issues persisted. I don't really want to enter in a rabbit hole trying to fix these issues that are unrelated to the changes in this PR, so I'm asking for another review. |
@rafaelzaleski, I was able to run the linting command locally and caught all the issues. Fix is in 08ae665. Also requesting a review from Proton as we're touching core files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I tested by switching currency to XPF and checked various flows on the shop instead of enabling multi-currency.
For this:
The fact that the total value is formatted differently in Blocks is a Blocks issue (Automattic/woocommerce-payments#8651). I left a comment flagging the same behavior for XPF here: Automattic/woocommerce-payments#8651 (comment).
Is there an open issue in the blocks somewhere? If so, does that needs to be fixed before we merge this PR?
Since multi-currency is a feature of WooPayments, that is being tracked in Automattic/woocommerce-payments#8651 and we'll handled it within the WooPayments scope. I don't think this blocks this PR once the impact is pretty low. |
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes Automattic/woocommerce-payments#5150.
The issue above suggests that we need to change how the XPF currency is formatted. Currently, the currency is displayed like
1 234 Fr
. The suggestion is to change it toXPF 1,234
. I did some investigation by looking up places online where I could see how they were presenting their currency and my conclusion is that we may want to adopt the suggestion as a default option, but not for the specific locales that use it.In French Polynesia
It's clear that the
XPF
symbol is predominant. The formatting varies a bit, but most places seem to use123 456 XPF
.In New Caledonia
The symbol used was more split in this one, but the preferred options still seems to be
XPF
. The formatting was more consistent, being123 456 XPF
the most used.In Wallis and Futuna
For this country I couldn't find examples of how they use the XPF currency. Therefore, I think it's safe to assume it's similar to the aforementioned countries until someone proves otherwise.
Conclusion
By navigating through some stores of these countries and seeing the examples, we can confirm that
XPF
is widely adopted. We still see some variations likeFr
,F
andFCFP
, but since these countries usually offer more than one currency,XPF
is more known and used among the options.To sum up the changes in this PR:
Fr
toXPF
.,
to.
. I did this because XPF doesn't have decimals, and if we're ever going to use comma, it's going to be in the thousand separator.XPF 1,234
.How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Pre-requisites
Payments > Settings
.pnpm --filter='@woocommerce/plugin-woocommerce' build:zip
.5.1. If you installed it in an existing site, you will need to clear up the following transients from the
wp_options
table. That way we ensure we have the most up-to-date locale and currency information on WooPayments.-
_transient_wcpay_currency_format
-
_transient_wcpay_locale_info
Test: XPF currency is formatted correctly in the currency preview
WooCommerce > Settings > Multi-currency
.XPF
currency, if needed.XPF1,234
.Test: XPF currency is formatted correctly in the shortcode cart/checkout
XPF 1,234
. Note there's a space between the symbol and the number.Test: XPF currency is formatted correctly in the cart/checkout Blocks
XPF1,234 XPF
.Note
The fact that the total value is formatted differently in Blocks is a Blocks issue (Automattic/woocommerce-payments#8651). I left a comment flagging the same behavior for XPF here: Automattic/woocommerce-payments#8651 (comment).
Changelog entry
Significance
Type
Message
Comment