i18n: format bytes with consistent fractional width #11489

acutmore · 2020-09-29T13:05:03Z

Summary
This changes how numbers with the 'byte' itemType are formatted when viewed.

Previously 2.5 and 1 when formatted with a granularity of 0.1 would be shown as 2.5 KiB and 1 KiB. Now they would be formatted as 2.5 KiB and 1.0 KiB.

This consistency can make it easier for some people when comparing sizes in a column as the decimal places will be aligned.

Related Issues/PRs

Fixes #11422

acutmore · 2020-09-29T13:06:21Z

lighthouse-core/report/html/renderer/i18n.js

    return `${kbs}${NBSP2}KiB`;
  }

  /**
   * @param {number} size
-   * @param {number=} granularity Controls how coarse the displayed value is, defaults to 0.1


correcting this little typo while I was in the area

acutmore · 2020-09-29T13:10:37Z

lighthouse-core/report/html/renderer/i18n.js

+    if (granularity >= 1) {
+      return this._numberFormatter;
+    }
+    const numberOfFractionDigits = -Math.floor(Math.log10(granularity));


Having just Math.log10 on its own might be sufficient, the added Math.floor allows for granularities like 0.2 to also work.

lighthouse-core/test/report/html/renderer/i18n-test.js

acutmore · 2020-09-30T13:40:57Z

'smoke_2_ToT' failed when testing http://localhost:10200/oopif.html with

"The page loaded too slowly to finish within the time limit. Results may be incomplete."

Presuming this means there is a chance these changes have introduced a performance regression. Might be that creating lots of new Intl.NumberFormat is more expensive than I had pressured. One idea would be to cache them?

brendankenny · 2020-09-30T13:57:20Z

That's almost certainly just a test flake, see #11341.

brendankenny

This looks great! Just a few notes.

Honestly this PR makes me think this._numberFormatter shouldn't be saved at all, since all of these format*() functions also have a granularity param. But that can wait for a future PR :)

brendankenny · 2020-09-30T16:37:28Z

lighthouse-core/test/report/html/renderer/report-ui-features-test.js

-          '/script3.js(www.notexample.com)24 KiB8.8 KiB',
-          '50 KiB0 KiB',
-          '60 KiB0 KiB',
+          '/script1.js(www.cdn.com)24.0 KiB8.8 KiB',


there are probably existing cases in the report where we don't want the extra .0 (like where the audit has already rounded to the nearest integer), but hopefully that will encourage audits to use granularity when that's what they really need :)

Very good point. I was also wondering whether to special case zero, keeping it as 0 KiB rather than 0.0 KiB.

brendankenny · 2020-09-30T17:06:09Z

lighthouse-core/report/html/renderer/i18n.js

+  _byteFormatterForGranularity(granularity) {
+    let numberOfFractionDigits = 0;
+    if (granularity < 1) {
+      // assumes granularities above 1 will not contain fractional parts, i.e. will never be 1.5


is this an old comment? granularity > 1 will never get inside this conditional

I probably need to re-word/move this comment. The assumption is related to the if its inside of, only checking the fractional part of the granularity if it is below 1.
EDIT: I've moved the comment up out of the if in 930c176. Hoping that makes things a bit clearer.

brendankenny · 2020-09-30T17:23:11Z

lighthouse-core/report/html/renderer/i18n.js

+    return new Intl.NumberFormat(this._numberDateLocale, {
+      maximumFractionDigits: numberOfFractionDigits,
+      minimumFractionDigits: numberOfFractionDigits,
+    });


Just in case some of the other formatting options get changed up in the constructor, we should use them here, too. I don't know why someone might change maximumSignificantDigits or something, but better to keep things robust :)

Something like

Suggested change

return new Intl.NumberFormat(this._numberDateLocale, {

maximumFractionDigits: numberOfFractionDigits,

minimumFractionDigits: numberOfFractionDigits,

});

return new Intl.NumberFormat(this._numberDateLocale, {

...this._numberDateLocale.resolvedOptions(),

maximumFractionDigits: numberOfFractionDigits,

minimumFractionDigits: numberOfFractionDigits,

});

should work correctly.

oops, sorry, that should have been this._numberFormatter.resolvedOptions()

brendankenny · 2020-09-30T17:30:07Z

lighthouse-core/test/report/html/renderer/i18n-test.js

@@ -89,4 +89,31 @@ describe('util helpers', () => {
      timestamp.includes('Apr 29, 2017')
    );
  });
+
+  describe('_byteFormatterForGranularity', () => {


rather than testing the internal implementation, can you move these tests to using the external i18n.formatBytesToKiB (which is less likely to change in the future)? That would also allow exercising the rounding part, so e.g. granularity: 0.5 really would be tested as rounding to 0.5.

Sure thing! I'll push a change for this asap.
EDIT: pushed d44662d

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

acutmore · 2020-10-03T13:41:54Z

rebased from lighthouseVersion 6.4.0 onto lighthouseVersion 6.4.1

googlebot · 2020-10-03T14:38:34Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

brendankenny

this is great, thanks @acutmore!

acutmore · 2020-10-03T19:31:56Z

thanks @brendankenny. Also as a first time contributor to Lighthouse just wanted to say project documentation was really good. All very clear about how to get setup, run the tests locally and contribute a PR.

acutmore requested a review from a team as a code owner September 29, 2020 13:05

acutmore requested review from brendankenny and removed request for a team September 29, 2020 13:05

googlebot added the cla: yes label Sep 29, 2020

vercel bot deployed to Preview September 29, 2020 13:05 View deployment

acutmore commented Sep 29, 2020

View reviewed changes

devtools-bot assigned brendankenny Sep 29, 2020

devtools-bot added the waiting4reviewer label Sep 29, 2020

acutmore commented Sep 29, 2020

View reviewed changes

connorjclark reviewed Sep 29, 2020

View reviewed changes

lighthouse-core/test/report/html/renderer/i18n-test.js Show resolved Hide resolved

vercel bot deployed to Preview September 30, 2020 11:16 View deployment

brendankenny requested changes Sep 30, 2020

View reviewed changes

brendankenny added waiting4committer and removed waiting4reviewer labels Sep 30, 2020

vercel bot deployed to Preview September 30, 2020 18:16 View deployment

vercel bot deployed to Preview October 3, 2020 12:32 View deployment

acutmore and others added 5 commits October 3, 2020 14:38

i18n: format bytes with consistent fractional width

f02d7a5

tests(i18n): direct test for _byteFormatterForGranularity

521bc32

apply all options from existing _numberDateLocale

9f68cfc

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

fix typo from 9f68cfc

930c176

unit tests now only use the external interface

d44662d

vercel bot deployed to Preview October 3, 2020 13:40 View deployment

This comment has been minimized.

Sign in to view

googlebot added cla: no and removed cla: yes labels Oct 3, 2020

acutmore requested a review from brendankenny October 3, 2020 14:35

brendankenny removed the cla: no label Oct 3, 2020

brendankenny added the cla: yes label Oct 3, 2020

brendankenny approved these changes Oct 3, 2020

View reviewed changes

brendankenny added the land-when-ci-is-green label Oct 3, 2020

devtools-bot merged commit a79df8b into GoogleChrome:master Oct 3, 2020

devtools-bot removed the land-when-ci-is-green label Oct 3, 2020

brendankenny mentioned this pull request Apr 7, 2022

i18n: localize units in report formatter #13830

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i18n: format bytes with consistent fractional width #11489

i18n: format bytes with consistent fractional width #11489

acutmore commented Sep 29, 2020

acutmore Sep 29, 2020

acutmore Sep 29, 2020 •

edited

Loading

acutmore commented Sep 30, 2020

brendankenny commented Sep 30, 2020

brendankenny left a comment

brendankenny Sep 30, 2020

acutmore Sep 30, 2020

brendankenny Sep 30, 2020

acutmore Sep 30, 2020 •

edited

Loading

brendankenny Sep 30, 2020

brendankenny Sep 30, 2020

brendankenny Sep 30, 2020

acutmore Sep 30, 2020 •

edited

Loading

This comment has been minimized.

acutmore commented Oct 3, 2020

googlebot commented Oct 3, 2020

brendankenny left a comment

acutmore commented Oct 3, 2020

i18n: format bytes with consistent fractional width #11489

i18n: format bytes with consistent fractional width #11489

Conversation

acutmore commented Sep 29, 2020

acutmore Sep 29, 2020

Choose a reason for hiding this comment

acutmore Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

acutmore commented Sep 30, 2020

brendankenny commented Sep 30, 2020

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Sep 30, 2020

Choose a reason for hiding this comment

acutmore Sep 30, 2020

Choose a reason for hiding this comment

brendankenny Sep 30, 2020

Choose a reason for hiding this comment

acutmore Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

brendankenny Sep 30, 2020

Choose a reason for hiding this comment

brendankenny Sep 30, 2020

Choose a reason for hiding this comment

brendankenny Sep 30, 2020

Choose a reason for hiding this comment

acutmore Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

acutmore commented Oct 3, 2020

googlebot commented Oct 3, 2020

brendankenny left a comment

Choose a reason for hiding this comment

acutmore commented Oct 3, 2020

acutmore Sep 29, 2020 •

edited

Loading

acutmore Sep 30, 2020 •

edited

Loading

acutmore Sep 30, 2020 •

edited

Loading