Site-kit-wp: The formatting of numbers followed by % signs can be different per language

[ryanwelcher]

Feature Description

Depending on the language, a number may or may not have a space preceding a % symbol. For example, English is 50% while many other languages would display as 50 %.

Anywhere Site Kit displays a number followed by a % symbol should be marked for translation

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• All percent and currency numbers, shown on the admin pages, should be formatted according to the current locale. Special attention should be paid to the languages that use other formatting conventions for numbers and punctuations (like in France) or RTL languages.
• When the user changes languages in the settings, the formatting number should also change accordingly.

Implementation Brief

• Add a new more Site Kit-specific opinionated version of numberFormat, which has the same signature and internally wraps numberFormat, but provides defaults according to Site Kit formatting standards.
• Add a new possible value for style, which is metric. In this case, the new function should rely on the existing readableLargeNumber to format the number. This metric value should also be the default style for the new function.
• Allow a shorthand for the options parameter: Passing % should format the value as percentage, passing s should format the value as seconds / duration, passing any other string should be interpreted as if this string was a currency code. This allows simple usages like numFmt( 0.23, '%' ), numFmt( 182, 's' ) or numFmt( 9.99, 'USD' ).
• For percent, the function should expect a number between 0 and 1, like common elsewhere and numberFormat as well. Anywhere where we actually use full percentage numbers outside (between 0 and 100), those need to be divided by 100 when passed to the formatting function.
• Here is a draft of that function:

function numFmt( number, options = {} ) { // Name TBD.
    // Expand shorthand values for options.
    if ( '%' === options ) {
        options = { style: 'percent' }
    } else if ( 's' === options ) {
        options = { style: 'seconds' }
    } else if ( typeof options === 'string' ) {
        options = { style: 'currency', currencyCode: options }
    }
    // Note: `metric` is our custom, default style.
    const { style = 'metric' } = options;
​
    if ( 'metric' === style ) {
        return readableLargeNumber( number );
    }
​
    if ( 'seconds' === style ) {
        return prepareSecondsForDisplay( number );
    }
​
    return numberFormat( number, options );
}

• The DataBlock component should be updated:
  
  
  
  • It should expect datapoint and change to be unformatted numbers. Any % or currency code should be passed as datapointUnit / changeDataUnit like originally intended.
  
  
  • The component should internally rely on the new function and the above shorthand syntax in case a "unit" is passed.
  
  
• All usages of the DataBlock component should be updated to pass raw unformatted numbers. Any potentially passed units should instead be passed via the separate prop.
• All remaining usages of readableLargeNumber should be replaced with the new function.
• All remaining usages of numberFormat should be replaced with the new function.
• The AcquisitionSources component should be updated to use the new function instead of using hardcoded formatting for mini chart values.
• The extractAnalyticsDashboardData function should be updated to use the new function as well instead of concatenating the number with a percent character.
• The readableLargeNumber function should have its second parameter removed. Its ability to format currencies is confusing and now no longer needed.
• Check the codebase to make sure all percentages and currencies are displaying correctly using the new function approach.

QA Brief

Please go through all the different screens of the plugin to check the formatting as below.

• Set up Site Kit on your WordPress instance using English locale.
• Make sure numbers are formatted correctly.
• Switch your site locale to French;
• Make sure that number formatting has been changed too and now percentages are showing with whitespace between number and percent symbol.
• Switch to an RTL language locale.
• Make sure numbers are formatted correctly according to the language conventions.

Changelog entry

• Introduce numFmt function for centralized Site Kit-specific number formatting, localize percentage formatting, and fix various number formatting inconsistencies.

[eugene-manuilov]

@felixarntz I have moved your acceptance criteria to the IB section and slightly updated/extended it. Added new acceptance criteria that explain the desired outcome of this issue. Hope it works for you.

[felixarntz]

IB ✅

[aaemnnosttv]

@felixarntz chiming in a bit late here – I'm not sure about the proposed new function; why not just use numberFormat with the appropriate options? numberFormatWithUnit as it is described is difficult to understand and unnecessary in most cases I think. unit has a special case for % but it may also be a currency code (which is not a unit). The fallback behavior of appending the unit at the end also does not seem very i18n-friendly which I understand is the primary reason for creating this in the first place.

For example, I think this change to readableLargeNumber is better the way it was before

Even though it's slightly longer, it's much clearer what it's doing.

I think most of the added complexity here is due to mixed prop values to DataBlock in which case, a function like this may simplify things in the component a bit, but I don't think it justifies adding a utility for this to use elsewhere. We could refactor DataBlock to use this function, but keep it within the scope of that component or module. IMO, we should update DataBlock to be more straightforward to use rather than introducing a new function that does too many things.

Apologies for the late input here, but I think an approach centered around using numberFormat would be better than introducing a variant of it which is less predictable and easy to understand.
cc: @eugene-manuilov

[felixarntz]

@kostyalmm Please see the new IB here, more info in https://github.com/google/site-kit-wp/pull/2451#issuecomment-742869836.

[kostyalmm]

@felixarntz the updated IB doesn't cover the case when the dataPoint(or change) is of type seconds. We currently have prepareSecondsForDisplay for displaying it. Shall we move the logic of prepareSecondsForDisplay to the new numFmt function?
This will make seconds another of our custom style.

[felixarntz]

@kostyalmm Ah, good catch! Yes, let's add support for that to numFmt as well. I think seconds makes sense for the style name, alternatively we could use duration, but that would be less precise in terms of what kind of number is expected.

We'll also need a "shorthand" version to support that. Let's use s for that, as it would be a similar approach to passing % or a currency code. That way, in the cases where prepareSecondsForDisplay is currently used with DataBlock, you'd then pass s as e.g. datapointUnit prop.

I've updated the IB to cover that.

[felixarntz]

Moving this to the following release as it will benefit from a lot of regular usage before shipping this, due to all of the potentially affected areas. This will be a major consistency win once ready though! 🎉

[tofumatt]

Based on https://github.com/google/site-kit-wp/pull/2451#issuecomment-747721592 I'm moving this back into Code Review, looks like @kostyalmm wanted you to have another review of his approach, @felixarntz.

But he won't be around next week so feel free to assign myself or @benbowler after review if more work is needed on this issue. Thanks! 🙂

[wpdarren]

QA Update: Pass ✅

Verified:

When English is the locale, the percentage appears after the number with no whitespace. - Screenshot
When French is the locale, the percentage appears after the number with whitespace - Screenshot
When an RTL (Arabic) is the local, the percentage appears after the number with no whitespace. - Screenshot

i also selected a few other locales to make sure that the number and percentages were displaying as expected.