-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Add numberFormatI18n Port to @wordpress/i18n
#71214
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
base: trunk
Are you sure you want to change the base?
Add numberFormatI18n Port to @wordpress/i18n
#71214
Conversation
@wordpress/i18n
@wordpress/i18n
packages/i18n/src/create-i18n.ts
Outdated
const wpLocale = localeData.lang || 'en_US'; | ||
let jsLocale; | ||
if ( ! I18N_WP_LOCALE_REGEXP.test( wpLocale ) ) { | ||
// If the locale is not in the expected format, default to 'en-US'. | ||
jsLocale = 'en-US'; | ||
} else { | ||
jsLocale = wpLocale.replace( '_', '-' ); | ||
} |
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.
Let's make sure this works for all the locales listed at https://make.wordpress.org/polyglots/teams/
I just tested them with Intl.getCanonicalLocales()
and here are my findings:
de-DE-formal
are actually supported (I thought it wasn't), so we should make it work too.art-xemoji
is supported toopt-PT-ao90
doesn't work, andIntl.getCanonicalLocales()
throws
So a regex is too overkill, maybe simply replacing the underscores is enough. And then perhaps try { Intl.getCanonicalLocales() } catch ...
to prevent errors.
We should have tests for such "special" locales too.
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.
Oh, thanks for informing, updating to reflect this.
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.
pt-PT-ao90 doesn't work, and Intl.getCanonicalLocales() throws
Seems likes this is not yet supported in both browsers and Node.js Intl so either we have to make it fallback to en-US
( current behaviour ) or write an if statement to make it fallback to pt-PT
.
Other than that, I have changed to use getCanonicalLocales() to test locale and I have updated the tests to include both special locales and all the wp Locales mentioned in make.wordpress.org/polyglots/teams
cc: @swissspidy
- added tests for special locales and all polygot locales - switched form regex to a getCanonicalLocale() test to check locale validity
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@wordpress/i18n
@wordpress/i18n
Co-authored-by: Pascal Birchler <[email protected]>
/** | ||
* Range limit of `maximumFractionDigits` in Node.js is 0-20 | ||
* Max limit of `maximumFractionDigits` in Browsers is 0-100 ( {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/NumberFormat#maximumfractiondigits} ) | ||
* | ||
* Hence we limit the decimals to 20 to ensure compatibility across environments. | ||
*/ | ||
const validDecimals = Math.max( 0, Math.min( decimals, 20 ) ); |
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.
What's the motivation for this? Is it really our responsibility to clamp this value? Seems more like the consumer's job.
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 reason behind this was that during testing I found out that while Browser supported upto 100 decimals , Nodejs was limited to around 20 decimal points and anything above that it would throw an error saying invalid range.
Hence I thought of adding clamp to this to prevent this error.
What?
Closes #22628
This PR adds
numberFormatI18n
function to the@wordpress/i18n
package that formats numbers according to the current locale settings, providing a JavaScript equivalent to WordPress's PHPnumber_format_i18n
functionWhy?
WordPress translation functions have had
number_format_i18n
for formatting numbers in a locale-appropriate way for some time. This functionality was missing from the JavaScript@wordpress/i18n
package, making it difficult for developers to format numbers consistently with the site's locale in JavaScript/React components.How?
The implementation adds a
numberFormatI18n
function that:lang
property from existing locale data rather than adding dependencies on other packagesde_DE
) to JavaScript BCP 47 format (e.g.,de-DE
)en-US
for invalid or unrecognized localesSyntax
Testing Instructions
Unit Tests for this function in
create-i18n
test file should suffice.Screenshots or screencast