core(displayvalue): change displayValue to be string only #6767
Conversation
exterkamp
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
exterkamp
changed the title
| @@ -265,8 +279,9 @@ class FontSize extends Audit { | |||
| }); | |||
| } | |||
|
|
|||
| const percentFormatted = (percentageOfPassingText / 100).toFixed(4); | |||
There was a problem hiding this comment.
wait hang on, triple sorry for this I seem to be all over the place with this PR 😭
isn't this already going to be a string with 4 sigFigs before it ever gets to the i18n bit so all of what I've said is going to be moot anyhow?
There was a problem hiding this comment.
So I make it guaranteed to be 4 digits only, but not necessarily 4 sigfigs e.g. 0.0318 -> 3.18% which is 2 decimal places after the . but is only 3 sig figs. So I guess maybe I should rename percent4sigfig to percent2trailing because it will clip off any fraction with more than 2 decimal places. Technically I don't think I need to format this to 4 digits here since Intl will do that with the 4 max digits anyway.
There was a problem hiding this comment.
OK how's about this.
The (percentageOfPassingText / 100).toFixed(4) is super clear to me. Will i18n lib avoid a custom formatting option entirely if we can just pass that directly as just 'percent' styled?
There was a problem hiding this comment.
using style:percent will convert 0.XXYY -> XX%
docs
The minimum number of fraction digits to use. Possible values are from 0 to 20; the default for plain number and percent formatting is 0;
the default for percent formatting is the larger of minimumFractionDigits and 0.
So the default is 0 fractional digits. Which is why I set it to four* two to make it force have two trailing digits. In order to have more digits we have to send it those additional options.
*I was mistaken, it applies this # of digits after the x100, so 2 is the actual final number of trailing digits.
|
Okay I think the naming conventions are in a good spot. |
| 'visitors to “pinch to zoom” in order to read. Strive to have >60% of page text ≥12px. ' + | ||
| '[Learn more](https://developers.google.com/web/tools/lighthouse/audits/font-sizes).', | ||
| /** [ICU Syntax] Label for the audit identifying font sizes that are too small. */ | ||
| displayValue: '{decimalProportion, number, extendedPercent} legible text', |
There was a problem hiding this comment.
remember on updating these in the console we'll have to add a special extractor/replacer for extendedPercent like we've added for bytes and milliseconds
| /** @type {LH.Audit.DisplayValue} */ | ||
| const displayValue = ['%.1d% legible text', percentageOfPassingText]; | ||
| const displayValue = str_(UIStrings.displayValue, {decimalProportion}); |
There was a problem hiding this comment.
it's weird that we have displayValue and explanation here, they're subtly different but mostly the same, and explanation isn't localized. I don't see any localized explanations in any other audit, though.
(Really seems like there should only be an explanation here if the disclaimer is defined...otherwise it's just a repeat of the displayValue. I recognize this is all out of scope, though :)
| @@ -71,6 +71,11 @@ const formats = { | |||
| minimumFractionDigits: 1, | |||
| maximumFractionDigits: 1, | |||
| }, | |||
| extendedPercent: { | |||
| // Force allow up to two digits after decimal place in percentages. (Intl.NumberFormat options) | |||
| maximumFractionDigits: 2, | |||
There was a problem hiding this comment.
this makes way more sense :)
| @@ -76,4 +76,47 @@ describe('i18n', () => { | |||
| expect(i18n.lookupLocale('jk-Latn-DE-1996-a-ext-x-phonebk-i-klingon')).toEqual('en'); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('#_formatIcuMessage', () => { | |||
Summary
Convert displayValue to be a
stringvalue only instead of[string, <string | number> ]for consistency. Also converted the only 2 audits that still use the old displayValue array format.Related Issues/PRs
fixes: #6200