i18n: localize units in report formatter #13830
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,25 +22,58 @@ export class I18n { | |
| // When testing, use a locale with more exciting numeric formatting. | ||
| if (locale === 'en-XA') locale = 'de'; | ||
|
|
||
| this._locale = locale; | ||
| this._strings = strings; | ||
| } | ||
|
|
||
| get strings() { | ||
| return this._strings; | ||
| } | ||
|
|
||
| /** | ||
| * @param {number} number | ||
| * @param {number} granularity | ||
| * @param {Intl.NumberFormatOptions} opts | ||
| * @return {string} | ||
| */ | ||
| _formatNumberWithGranularity(number, granularity, opts = {}) { | ||
| opts = {...opts}; | ||
This comment was marked as resolved.
Sorry, something went wrong. There was a problem hiding this comment. yeah i had my cs prof yelling in my head "don't mutate input parameters!" so i copied it |
||
| const log10 = -Math.log10(granularity); | ||
| if (!Number.isFinite(log10) || (granularity > 1 && Math.floor(log10) !== log10)) { | ||
|
There was a problem hiding this comment. I think There was a problem hiding this comment. We have a test that uses 0.5 as a value here: and given we pass hmmm throwing in report code is probably bad anyhow. wish we had a DCHECK (cc @exterkamp lol) There was a problem hiding this comment.
ha, well that looks like something that was supported so it was nice to add a test, rather that something we've ever used or wanted to use, but if we want to keep it, There was a problem hiding this comment. for some reason i had no idea |
||
| throw new Error(`granularity of ${granularity} is invalid`); | ||
| } | ||
|
|
||
| if (granularity < 1) { | ||
| opts.minimumFractionDigits = opts.maximumFractionDigits = Math.ceil(log10); | ||
| } | ||
|
|
||
| number = Math.round(number / granularity) * granularity; | ||
|
|
||
| // Avoid displaying a negative value that rounds to zero as "0". | ||
|
There was a problem hiding this comment.
|
||
| if (Object.is(number, -0)) number = 0; | ||
|
There was a problem hiding this comment. I could have done There was a problem hiding this comment. What if JS added There was a problem hiding this comment. IEEE-754 requires that they compare as equal 🤓 |
||
|
|
||
| return new Intl.NumberFormat(this._locale, opts).format(number).replace(' ', NBSP2); | ||
|
There was a problem hiding this comment. It seems like it should be fine to do the replace thing, though it feels dirty and maybe there's somehow an issue with rtl or something? 🤷 Do we still need it? I assume we do, but it would be cool if our various layout initiatives over the years made it unnecessary to keep them from breaking across lines There was a problem hiding this comment. Interestingly, for
Also, languages disagree on spacing of units :/ There was a problem hiding this comment. omg this is infuriatingly inconsistent. no way this is due to locale differences, these control characters are formatting and what locale would expect numbers to be disassociated from their unit by newlines?? this has to be a mistake in the localization library... your One can sort of see reason for there being a non-breaking space between the number and the short string but not for the longer one, as without that number the shorter strings being further from their number gives them far less context. (your last example is the same two lines of code btw. i'd expect narrow to never include any whitespace. I see There was a problem hiding this comment.
totally, but why isn't it the same across languages?? Or at least, why wouldn't english (or german or spanish or...) do it as well?
oops, I meant
There was a problem hiding this comment.
hmm.... |
||
| } | ||
|
|
||
| /** | ||
| * Format number. | ||
| * @param {number} number | ||
| * @param {number=} granularity Number of decimal places to include. Defaults to 0.1. | ||
| * @return {string} | ||
| */ | ||
| formatNumber(number, granularity = 0.1) { | ||
|
There was a problem hiding this comment. perhaps this default should be 1? it would match the previous behavior, such that integers are formatted without a fractional component There was a problem hiding this comment. instead I made a new function formatInteger There was a problem hiding this comment. yeah, it's reasonable to show the extra fractional digit when granularity is 0.1 (which is what #11489 did for bytes), and maybe we don't really want 0.1 as the default based on how we usually don't do anything about it, but that's annoying to audit and |
||
| return this._formatNumberWithGranularity(number, granularity); | ||
| } | ||
|
|
||
| /** | ||
| * Format integer. | ||
| * Just like {@link formatNumber} but uses a granularity of 1, rounding to the nearest | ||
| * whole number. | ||
| * @param {number} number | ||
| * @return {string} | ||
| */ | ||
| formatInteger(number) { | ||
| return this._formatNumberWithGranularity(number, 1); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -49,7 +82,7 @@ export class I18n { | |
| * @return {string} | ||
| */ | ||
| formatPercent(number) { | ||
| return new Intl.NumberFormat(this._locale, {style: 'percent'}).format(number); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -58,9 +91,7 @@ export class I18n { | |
| * @return {string} | ||
| */ | ||
| formatBytesToKiB(size, granularity = 0.1) { | ||
| return this._formatNumberWithGranularity(size / KiB, granularity) + `${NBSP2}KiB`; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -69,9 +100,7 @@ export class I18n { | |
| * @return {string} | ||
| */ | ||
| formatBytesToMiB(size, granularity = 0.1) { | ||
| return this._formatNumberWithGranularity(size / MiB, granularity) + `${NBSP2}MiB`; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -80,9 +109,11 @@ export class I18n { | |
| * @return {string} | ||
| */ | ||
| formatBytes(size, granularity = 1) { | ||
| return this._formatNumberWithGranularity(size, granularity, { | ||
| style: 'unit', | ||
| unit: 'byte', | ||
| unitDisplay: 'long', | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -93,25 +124,10 @@ export class I18n { | |
| formatBytesWithBestUnit(size, granularity = 0.1) { | ||
| if (size >= MiB) return this.formatBytesToMiB(size, granularity); | ||
| if (size >= KiB) return this.formatBytesToKiB(size, granularity); | ||
| return this._formatNumberWithGranularity(size, granularity, { | ||
| style: 'unit', | ||
| unit: 'byte', | ||
| unitDisplay: 'narrow', | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -121,10 +137,11 @@ export class I18n { | |
| * @return {string} | ||
| */ | ||
| formatMilliseconds(ms, granularity = 10) { | ||
| return this._formatNumberWithGranularity(ms, granularity, { | ||
| style: 'unit', | ||
| unit: 'millisecond', | ||
| unitDisplay: 'short', | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -133,8 +150,11 @@ export class I18n { | |
| * @return {string} | ||
| */ | ||
| formatSeconds(ms, granularity = 0.1) { | ||
| return this._formatNumberWithGranularity(ms / 1000, granularity, { | ||
| style: 'unit', | ||
| unit: 'second', | ||
| unitDisplay: 'short', | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -154,10 +174,10 @@ export class I18n { | |
| // and https://github.com/GoogleChrome/lighthouse/pull/9822 | ||
| let formatter; | ||
| try { | ||
| formatter = new Intl.DateTimeFormat(this._locale, options); | ||
| } catch (err) { | ||
| options.timeZone = 'UTC'; | ||
| formatter = new Intl.DateTimeFormat(this._locale, options); | ||
| } | ||
|
|
||
| return formatter.format(new Date(date)); | ||
|
|
@@ -169,6 +189,10 @@ export class I18n { | |
| * @return {string} | ||
| */ | ||
| formatDuration(timeInMilliseconds) { | ||
|
There was a problem hiding this comment. just realized the comment for this function was wrong
we had NBSP all up in there... |
||
| // There is a proposal for a Intl.DurationFormat. | ||
| // https://github.com/tc39/proposal-intl-duration-format | ||
| // Until then, we do things a bit more manually. | ||
|
|
||
| let timeInSeconds = timeInMilliseconds / 1000; | ||
| if (Math.round(timeInSeconds) === 0) { | ||
| return 'None'; | ||
|
|
@@ -177,19 +201,33 @@ export class I18n { | |
| /** @type {Array<string>} */ | ||
| const parts = []; | ||
| /** @type {Record<string, number>} */ | ||
| const unitToSecondsPer = { | ||
| day: 60 * 60 * 24, | ||
| hour: 60 * 60, | ||
| minute: 60, | ||
| second: 1, | ||
| }; | ||
| /** @type {Record<string, string>} */ | ||
| const unitToDefaultLabel = { | ||
| day: 'd', | ||
| hour: 'h', | ||
| minute: 'm', | ||
| second: 's', | ||
| }; | ||
|
|
||
| Object.keys(unitToSecondsPer).forEach(unit => { | ||
| const unitFormatter = new Intl.NumberFormat(this._locale, { | ||
| style: 'unit', | ||
| unit, | ||
| unitDisplay: 'narrow', | ||
| }); | ||
| const label = unitFormatter.formatToParts(0) | ||
|
There was a problem hiding this comment. this seems like it could be problematic to use 0 here for languages with different (abbreviated) unit names for plurals? Might not be a problem for any of our languages, but this also seems to be a very roundabout way vs concatenating calls to There was a problem hiding this comment. good point, better to call that function directly |
||
| .find(p => p.type === 'unit')?.value || unitToDefaultLabel[unit]; | ||
| const secondsPerUnit = unitToSecondsPer[unit]; | ||
| const numberOfUnits = Math.floor(timeInSeconds / secondsPerUnit); | ||
| if (numberOfUnits > 0) { | ||
| timeInSeconds -= numberOfUnits * secondsPerUnit; | ||
| parts.push(`${numberOfUnits}${NBSP2}${label}`); | ||
| } | ||
| }); | ||
|
|
||
|
|
||
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.
can do
unit: 'kilobit-per-second'for these since it's Kbps (not...Kibps?). It'll bekb/sinstead ofKbpsthoughThere 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.
sounds better to me!