1723 Adopt to locale string for all our number output #2553

arturmiz · 2017-06-20T21:04:47Z

Hi, I have come across this beginner issue, did some debugging how it can be fixed and I noticed that there are already some changes introduced with #2295. I followed the pattern and used this utility helper across audits which I think required use of it. Waiting for your feedback, thanks! 😅

patrickhulce

Thanks for this consistency improvement @arturmiz, great first contribution!

patrickhulce · 2017-06-20T21:24:18Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

@@ -34,7 +35,7 @@ class UnusedBytes extends Audit {
   * @return {string}
   */
  static bytesToKbString(bytes) {
-    return Math.round(bytes / KB_IN_BYTES).toLocaleString() + ' KB';
+    return `${Util.formateBytesToKB(bytes)} KB`;


you inherited this typo but do you want to update it to formatBytesToKB while you're in here? :)

Ok, I'll add the 'KB' inside this helper, similar how it's done now for milliseconds. One question though as formatMilliseconds helper looks:

return `${coarseTime.toLocaleString()}${NBSP}ms`;

Which (at least on my machine) renders to sth like 1,000ms so there is no space between number and ms. Should I done it the same way or just ad KB at the end for formatBytestoKB?

let's keep the nonbreaking space for now, cc: @paulirish for the final decision, but it's supposed to be a happy medium between a full space and no-space on most machines

patrickhulce · 2017-06-20T21:26:16Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

@@ -54,7 +55,7 @@ class UnusedBytes extends Audit {
   * @return {string}
   */
  static bytesToMsString(bytes, networkThroughput) {
-    return (Math.round(bytes / networkThroughput * 100) * 10).toLocaleString() + 'ms';
+    return Util.formatMilliseconds( Math.round(bytes / networkThroughput * 100) * 10, 1);


I think you could reduce this to just formatMilliseconds(bytes / networkThroughout * 1000)

patrickhulce · 2017-06-20T21:28:30Z

lighthouse-core/audits/dobetterweb/link-blocking-first-paint.js

      return {
        url: item.tag.url,
-        totalKb: `${Math.round(item.transferSize / 1024)} KB`,
-        totalMs: `${Math.round((item.endTime - startTime) * 1000)}ms`
+        totalKb: `${transferSizeKbs} KB`,


could do this inline and remove the Math.round for just formatBytesToKB(item.transferSize, 0)

patrickhulce · 2017-06-20T21:32:54Z

lighthouse-core/report/v2/renderer/util.js

@@ -35,15 +35,16 @@ class Util {
  /**
   * Format number.
   * @param {number} number
+   * @param {number} decimalPlaces Number of decimal places to include. Defaults to 1.


nit: number= since it's optional

patrickhulce · 2017-06-20T21:33:21Z

lighthouse-core/report/v2/renderer/util.js

  }

  /**
   * @param {number} size
-   * @param {number=} decimalPlaces Number of decimal places to include. Defaults to 2.
+   * @param {number} decimalPlaces Number of decimal places to include. Defaults to 2.


keep the = here since it's still optional

Oh, my bad, I thought it's a typo that's why I removed it... 😀

haha, no worries! here's a quick guide to the closure types in case you want to decipher what all these comments mean :)

patrickhulce · 2017-06-20T21:33:27Z

lighthouse-core/report/v2/renderer/util.js

@@ -52,7 +53,7 @@ class Util {

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


…calculation

codecov · 2017-06-26T23:14:52Z

Codecov Report

Merging #2553 into master will increase coverage by 0.74%.
The diff coverage is 87.87%.

@@            Coverage Diff             @@
##           master    #2553      +/-   ##
==========================================
+ Coverage   84.52%   85.26%   +0.74%     
==========================================
  Files         169      178       +9     
  Lines        5614     5835     +221     
  Branches      774      815      +41     
==========================================
+ Hits         4745     4975     +230     
+ Misses        855      833      -22     
- Partials       14       27      +13

Impacted Files	Coverage Δ
lighthouse-core/audits/estimated-input-latency.js	`95.65% <ø> (ø)`	⬆️
lighthouse-core/audits/load-fast-enough-for-pwa.js	`97.61% <ø> (ø)`	⬆️
lighthouse-core/audits/first-meaningful-paint.js	`93.75% <ø> (ø)`	⬆️
lighthouse-core/audits/is-on-https.js	`100% <100%> (ø)`	⬆️
...se-core/report/v2/renderer/crc-details-renderer.js	`95.83% <100%> (ø)`	⬆️
lighthouse-core/audits/deprecations.js	`100% <100%> (ø)`	⬆️
...re/audits/byte-efficiency/byte-efficiency-audit.js	`84.61% <100%> (+0.3%)`	⬆️
lighthouse-core/report/v2/renderer/util.js	`93.44% <100%> (+0.1%)`	⬆️
...thouse-core/report/v2/renderer/details-renderer.js	`89.28% <100%> (ø)`	⬆️
lighthouse-core/audits/dobetterweb/dom-size.js	`100% <100%> (ø)`	⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfcbc30...05b60f0. Read the comment docs.

paulirish · 2017-06-26T23:18:42Z

cc @stevepeak btw it came back ⬆️ ⬆️ ⬆️ ⬆️

our .codecov.yml

patrickhulce · 2017-06-27T02:02:59Z

@arturmiz are you ready for a re-review? :)

stevepeak · 2017-06-27T06:00:23Z

@paulirish looks like the yaml does not exist on this specific commit: https://github.com/arturmiz/lighthouse/blob/329d5a7ab1783f14997fed7e6618c5ac672796ae/.codecov.yml 404's. Codecov always uses the commit's yaml being tested. Thoughts on how to improve this?

arturmiz · 2017-06-27T07:40:15Z

@patrickhulce, anytime 😉

patrickhulce

looks good to me % nits, nicely done!

patrickhulce · 2017-06-27T17:25:19Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

@@ -54,7 +55,7 @@ class UnusedBytes extends Audit {
   * @return {string}
   */
  static bytesToMsString(bytes, networkThroughput) {
-    return (Math.round(bytes / networkThroughput * 100) * 10).toLocaleString() + 'ms';
+    return Util.formatMilliseconds(bytes / networkThroughput * 1000, 1);


let's keep the granularity at the default 10 here instead of 1

patrickhulce · 2017-06-27T17:26:39Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

@@ -34,7 +35,7 @@ class UnusedBytes extends Audit {
   * @return {string}
   */
  static bytesToKbString(bytes) {
-    return Math.round(bytes / KB_IN_BYTES).toLocaleString() + ' KB';
+    return Util.formatBytesToKB(bytes);


let's keep the round effect with 0 decimal places e.g. formatBytesToKB(bytes, 0)

Hm, are you sure? With 0 decimal places in some cases it will lead to misleading results e.g. instead of having 0.5 KB (50%) it will show 0 KB (50%).

most of these audits should be ignoring savings less than 2-4 KB have you seen some that don't? we should fix those separately :)

Ok, is it filtered on the audit level or somewhere further? If on audit level, then I can't see any such condition in this one so it returns everything. I found this example using some fake data in a unit test. I'll add it so you'll know during future fixing that this kind of result shouldn't be there.

patrickhulce · 2017-06-28T19:42:45Z

awesome thanks again @arturmiz! 🎉

arturmiz · 2017-06-29T12:13:01Z

Thanks for accepting my PR @patrickhulce ! Happy to help! 😄

paulirish · 2017-06-29T18:57:05Z

Thanks for taking on this bug, @arturmiz. We really appreciate it. 😁

arturmiz added 2 commits June 17, 2017 23:27

Replaced direct use of toLocaleString with proper util helpers

a6dde1c

Fixed missing formatting numbers as locale strings in audits #1723

00281ff

arturmiz mentioned this pull request Jun 20, 2017

Adopt toLocaleString for all our number output #1723

Closed

patrickhulce suggested changes Jun 20, 2017

View reviewed changes

paulirish assigned patrickhulce Jun 20, 2017

arturmiz added 4 commits June 26, 2017 22:56

Reverted incorrect JSDoc fixes

0c2562b

Improvements in util helper formatting bytes to KB

0a4f9e6

Unit tests and code fixes

d077465

Link blocking first paint audit - fixed incorrect argument for score …

329d5a7

…calculation

patrickhulce approved these changes Jun 27, 2017

View reviewed changes

Byte efficiency audit - formatting values precision fixes

05b60f0

patrickhulce merged commit 5c8d4ec into GoogleChrome:master Jun 28, 2017

arturmiz deleted the 1723-adopt-toLocaleString-for-all-our-number-output branch July 3, 2017 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1723 Adopt to locale string for all our number output #2553

1723 Adopt to locale string for all our number output #2553

arturmiz commented Jun 20, 2017 •

edited by brendankenny

Loading

patrickhulce left a comment

patrickhulce Jun 20, 2017

arturmiz Jun 22, 2017

patrickhulce Jun 22, 2017

patrickhulce Jun 20, 2017

patrickhulce Jun 20, 2017

patrickhulce Jun 20, 2017

patrickhulce Jun 20, 2017

arturmiz Jun 22, 2017

patrickhulce Jun 22, 2017

patrickhulce Jun 20, 2017

codecov bot commented Jun 26, 2017 •

edited

Loading

paulirish commented Jun 26, 2017

patrickhulce commented Jun 27, 2017

stevepeak commented Jun 27, 2017

arturmiz commented Jun 27, 2017

patrickhulce left a comment

patrickhulce Jun 27, 2017

patrickhulce Jun 27, 2017

arturmiz Jun 27, 2017

patrickhulce Jun 27, 2017

arturmiz Jun 28, 2017

patrickhulce commented Jun 28, 2017

arturmiz commented Jun 29, 2017

paulirish commented Jun 29, 2017

1723 Adopt to locale string for all our number output #2553

1723 Adopt to locale string for all our number output #2553

Conversation

arturmiz commented Jun 20, 2017 • edited by brendankenny Loading

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 26, 2017 • edited Loading

Codecov Report

paulirish commented Jun 26, 2017

patrickhulce commented Jun 27, 2017

stevepeak commented Jun 27, 2017

arturmiz commented Jun 27, 2017

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Jun 28, 2017

arturmiz commented Jun 29, 2017

paulirish commented Jun 29, 2017

arturmiz commented Jun 20, 2017 •

edited by brendankenny

Loading

codecov bot commented Jun 26, 2017 •

edited

Loading