Fix Adsense dash replace error and enhance formatted number i18n#1261
Fix Adsense dash replace error and enhance formatted number i18n#1261felixarntz merged 9 commits intodevelopfrom
Conversation
tofumatt
left a comment
There was a problem hiding this comment.
I like it, and I super-dig how many issues this fixes! 👍
The VRTs are failing, but that makes sense as this would have modified how things look. Once those are updated I'm happy with this approach, so approved assuming Travis passes 😄
|
Found the problem and fixed it. I did need to approve visual changes to one test where |
| // This line to make sure we use lower case local format, ex: en-us. | ||
| locale = locale.replace( '_', '-' ).toLocaleLowerCase(); | ||
| if ( siteKitLocale ) { | ||
| return siteKitLocale.replace( '_', '-' ); |
There was a problem hiding this comment.
Previously this was calling toLocaleLowerCase, but now it doesn't. Just wanna make sure this is working as expected. Do navigator.language and googlesitekit.locale have the same format?
There was a problem hiding this comment.
Yeah, I'm not sure why it was doing that. navigator.language is not fully lowercase, e.g. en-US although apparently some browsers like older iOS Safari return it in all lowercase. The docs show it used with both cases though as it comes from the property https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat/NumberFormat
There was a problem hiding this comment.
The only difference is that WP uses the underscore-separated locale and the browser uses a hyphen.
assets/js/util/index.js
Outdated
| if ( ! locale ) { | ||
| locale = navigator.language; | ||
| } | ||
| export const numberFormat = ( number, options = {}, locale = getLocale() ) => { |
There was a problem hiding this comment.
I wonder whether locale should instead be options.locale - the current signature feels a bit strange to me, especially since the locale is another optional parameter that has a sane default.
There was a problem hiding this comment.
It's mostly for testing I think. I don't think you'd ever want/need to pass the locale directly, but I don't see why it shouldn't be supported. I agree the signature is a bit odd as-is.
We could implement it like this, which doesn't look bad
export const numberFormat = ( number, options = {} ) => {
const { locale = getLocale(), ...formatOptions } = options;
return new Intl.NumberFormat( locale, formatOptions ).format( number );
};Edit: pushed! Take a look 😄
felixarntz
left a comment
There was a problem hiding this comment.
LGTM!
Happy to see you put de-DE as example, makes me feel at home 😆
Summary
Addresses issue #1092, #293, #1107
Relevant technical choices
getLocale()utility functionGets the user's chosen locale as defined in Site Kit's
googlesitekit.localeif available, falls back to the language set in the browsernumberFormatutility to accept an object of formatting options, and use the new locale helper for using the correct locale by defaultreadableLargeNumbernumberFormatand available options as much as possible rather than manual parsingDataBlockcomponents to usenumberFormatfor consistency when displaying change (should ideally take the change unit from props for formatting but this is still an improvement)E.g. using
de-DEusing
en-USChecklist