report: final metrics display, icons, whitespace polish #5130

paulirish · 2018-05-05T02:22:04Z

screenshots

example reports:

https://disastrous-bears.surge.sh/cnn.html
https://disastrous-bears.surge.sh/dbwtester.html
https://disastrous-bears.surge.sh/errors.html
https://disastrous-bears.surge.sh/example.html
https://disastrous-bears.surge.sh/paulirish.html
https://disastrous-bears.surge.sh/tinyhouse.html

https://disastrous-bears.surge.sh/z.devtools.cnn.html
https://disastrous-bears.surge.sh/z.devtools.dbwtester.html
https://disastrous-bears.surge.sh/z.devtools.errors.html
https://disastrous-bears.surge.sh/z.devtools.example.html
https://disastrous-bears.surge.sh/z.devtools.paulirish.html
https://disastrous-bears.surge.sh/z.devtools.tinyhouse.html

paulirish · 2018-05-05T02:23:40Z

ah this branch is lacking a rebase. let me take care of that before trying to review it. :/

vinamratasingal-zz · 2018-05-05T02:27:17Z

Looking good, LGTM on the UI side.

QQ: is it normal for the tooltip to lag a bit? i.e. I hover for a few seconds, and then the tooltip shows up?

patrickhulce

conflict party ahead 😭

patrickhulce · 2018-05-05T16:11:52Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

    const metricAudits = category.audits.filter(audit => audit.group === 'metrics');
    const metricAuditsEl = this.renderAuditGroup(groups['metrics'], {expandable: false});
+    const estValuesEl = this.dom.createChildOf(metricAuditsEl, 'div',
+    'lh-metrics__disclaimer lh-metrics__disclaimer--head');
+    estValuesEl.textContent = 'Values are estimated and may vary. ';


aw I liked Estimated values may vary, I guess it doesn't necessarily convey that all the values you see are estimated though 🤔

is the space at the end necessary? :)

patrickhulce · 2018-05-05T16:11:58Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

    const metricAudits = category.audits.filter(audit => audit.group === 'metrics');
    const metricAuditsEl = this.renderAuditGroup(groups['metrics'], {expandable: false});
+    const estValuesEl = this.dom.createChildOf(metricAuditsEl, 'div',
+    'lh-metrics__disclaimer lh-metrics__disclaimer--head');


nit: indent

paulirish · 2018-05-05T18:30:52Z

conflict party ahead 😭

all sorted now.

paulirish · 2018-05-05T21:52:47Z

@vinamratasingal

QQ: is it normal for the tooltip to lag a bit? i.e. I hover for a few seconds, and then the tooltip shows up?

I'm set them to be only show up after your cursor has been hovering there for 900ms. When the number is too low, you get unintentional tooltips.

I think the animation should take a little longer though. It's currently 150ms which is too fast given the long delay.

Of course, all these parameters are tweakable. I do agree the tooltips feel a little funny atm. Hopefully we can tweak them to success.

vinamratasingal-zz · 2018-05-05T21:59:10Z

@paulirish thanks for the explanation! SGTM on tweaking :)

patrickhulce

looks good overall! hoping we can shrink some lines with defaulting :)

patrickhulce · 2018-05-06T17:28:56Z

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

@@ -64,7 +64,7 @@ class CategoryRenderer {
      const tooltip = this.dom.createChildOf(textEl, 'div', 'lh-error-tooltip-content tooltip');
      tooltip.textContent = audit.result.debugString || 'Report error: no audit information';
    } else if (audit.result.debugString) {
-      const debugStrEl = auditEl.appendChild(this.dom.createElement('div', 'lh-debug'));
+      const debugStrEl = this.dom.createChildOf(titleEl, 'div', 'lh-debug');


drive by restructuring? seems unrelated

Yeah. Kinda separate but hey. I mentioned it on chat yesterday

before:

after:

patrickhulce · 2018-05-06T17:30:07Z

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

@@ -189,7 +190,7 @@ class CategoryRenderer {
    const notApplicableElem = this.renderAuditGroup({
      title: `Not applicable`,
    }, {expandable: true, itemCount: this._getTotalAuditsLength(elements)});
-    notApplicableElem.classList.add('lh-audit-group--notapplicable');
+    notApplicableElem.classList.add('lh-audit-group--adorned', 'lh-audit-group--notapplicable');


a11y audits are the only unadorned audit-groups now then?

a11y and seo audit groups don't get icons, yeah.

patrickhulce · 2018-05-06T17:31:21Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

+    try {
+      return audit.result.details.summary.wastedMs;
+    } catch (e) {
+      return -1;


should this return undefined? it doesn't seem like the undefined case is really handled well now though, this is only for errors anyhoo right? maybe it always returns wastedMs or MIN_VALUE to force it to bottom or something?

Yeah this is all about oppos that have errored.

I don't return undefined because of:

lighthouse/lighthouse-core/report/html/renderer/performance-category-renderer.js

Lines 153 to 154 in cc5434b

const wastedMsValues = opportunityAudits.map(audit => this._getWastedMs(audit));

const maxWaste = Math.max(...wastedMsValues);

min_value wfm!

patrickhulce · 2018-05-06T17:33:14Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

@@ -159,9 +177,11 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
      const groupEl = this.renderAuditGroup(groups['diagnostics'], {expandable: false});
      diagnosticAudits.forEach((item, i) => groupEl.appendChild(this.renderAudit(item, i)));
      groupEl.open = true;
+      groupEl.classList.add('lh-audit-group--adorned', 'lh-audit-group--diagnostics');


seems like the unadorned case is far more common, WDYT about it being the default to avoid this churn?

Not sure but i'll give it a shot.

patrickhulce · 2018-05-06T17:34:02Z

lighthouse-core/report/html/templates.html

@@ -425,7 +425,7 @@ <h1 class="leftnav__header__title">Lighthouse</h1>
 <template id="tmpl-lh-gauge">
  <style>
    .lh-gauge__wrapper {
-      --circle-size: calc(2.5 * var(--header-font-size));
+      --circle-size: calc(3 * var(--header-font-size));


IDK Paul I'm feelin' a multiple more like 2.7350215 ;)

Hwi and I spent ~2 mins tweaking this one. :)

We use base-font-size to scale everything so devtools is just tighter by changing like 1 number. As such, this sorta thing happens. but it seems to work out.

brendankenny · 2018-05-07T00:23:45Z

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

@@ -89,7 +89,7 @@ class CategoryRenderer {
   * @param {!ReportRenderer.CategoryJSON} category
   * @return {!Element}
   */
-  renderCategoryScore(category) {
+  renderCategoryHeader(category) {


brendankenny · 2018-05-07T00:27:48Z

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

@@ -113,22 +113,6 @@ describe('CategoryRenderer', () => {
    category.description = prevDesc;
  });

-  it('renders audits with debugString as failed', () => {


do we have a test for this that handles rendering of the new modes? e.g. I don't see an error one

tbh i'm not sure. :)

i'd like to do a followup once we have the 3 PRs landed (this one, scoreDisplayMode, and debugString -> warnings/errors/explanations) where we can try all the possibilities.

i'd like to do a followup

add to #5008?

paulirish · 2018-05-07T03:01:10Z

sg. done.

paulirish · 2018-05-07T19:12:20Z

@brendankenny @patrickhulce rebased. can you rereview since this is front of the rest?

paulirish · 2018-05-07T19:28:35Z

And FWIW #5151 #5137 #5136 are rebased against imaginary branches right now because the conflicts just got REAL and i'll have to do the squash merge technique for them. :)

brendankenny

some clarifying questions around Number.MIN_VALUE :)

brendankenny · 2018-05-07T19:43:09Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

@@ -86,18 +85,30 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
    return element;
  }

+  /**
+   * @param {!ReportRenderer.AuditJSON} audit
+   * @return {number|undefined}


not |undefined anymore

maybe add a comment why it's returning Number.MIN_VALUE? Seems bizarre even with context :)

brendankenny · 2018-05-07T19:46:13Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

+   */
+  _getWastedMs(audit) {
+    try {
+      return audit.result.details.summary.wastedMs;


whats the case where this won't be defined?

I can't think of one, but I'm okay adding a typeof check

like adding this on the line above?:

if (typeof audit.result.details.summary.wastedMs !== 'number') return Number.MIN_VALUE;

I can't think of one, but I'm okay adding a typeof check

oh, I'm just wondering how you end up in the catch block

don't opportunities with an error not have a summary block?
(looks like it to me)

ah, cool, so only in those cases? I was worried there were other weird cases where we were inconsistent. Maybe add a note that this is for fallback for errors?

ok, maybe a more explicit if (audit.result.details && audit.result.details.summary) {return audit.result.details.summary.wastedMs;} else { return Number.MIN_VALUE;} then?

brendankenny · 2018-05-07T19:46:36Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

    if (opportunityAudits.length) {
-      const maxWaste = Math.max(...opportunityAudits.map(audit => audit.result.rawValue));
+      const wastedMsValues = opportunityAudits.map(audit => this._getWastedMs(audit));
+      const maxWaste = Math.max(...wastedMsValues);


is it possible for this to be all Number.MIN_VALUEs?

yes. only 1 visible opportunity and it was an error.

but it works out:

well. i guess we'd eventually set elem.style.width = NaN%`

:)

my preference for "real fix" would be to explicitly handle error cases separately so we can assume they're all regular sane numbers

but for now, I have no strong preference other than clearly expressing a nonsense number in the error case, MIN_VALUE seemed more obviously nonsense then -1, if it creates too much other noise compared to -1 though, I'm fine with it

brendankenny

last suggestion to make more explicit (plus earlier comment about adding a comment), but otherwise LGTM

paulirish · 2018-05-07T21:48:19Z

thanks everyone for the reviews. appreciate it!

…#5130)

paulirish requested review from brendankenny, patrickhulce and vinamratasingal-zz as code owners May 5, 2018 02:22

vinamratasingal-zz approved these changes May 5, 2018

View reviewed changes

patrickhulce reviewed May 5, 2018

View reviewed changes

patrickhulce mentioned this pull request May 5, 2018

core(lhr): remove debugString, add explantion/errorMessage #5132

Merged

paulirish force-pushed the metricsdisplay branch from b0f0fce to e277c2e Compare May 5, 2018 18:27

paulirish mentioned this pull request May 6, 2018

report: DRY up audit & opportunity rendering #5136

Merged

paulirish force-pushed the metricsdisplay branch from 7b3e349 to cc5434b Compare May 6, 2018 03:20

patrickhulce suggested changes May 6, 2018

View reviewed changes

brendankenny reviewed May 7, 2018

View reviewed changes

paulirish force-pushed the metricsdisplay branch from edc1738 to 94edb8c Compare May 7, 2018 02:59

paulirish added 12 commits May 7, 2018 12:08

report: metrics display

b01434d

simple metrics disclaimer.

094ac84

updated filmstrip display

243177b

category description isn't guaranteed

2036ce7

first icons attempt.

a4c6b44

whitespace tweaks with hwi

8201509

more tweaks with hwi

2cfba17

refining.

f34698e

handle errored opps without a wastedMs

7bb3b54

metrics disclaimer

86e5271

tweaks.

91d1f63

move lh-debug within the audit

36ab00a

paulirish added 6 commits May 7, 2018 12:10

tweaks.

ec5ed49

tests.

0a579a1

alignment polish

6eda99d

no wrapping of metric values.

34d0e24

by default, audit groups are adorned.

ef57acd

MIN_VALUE

f668318

paulirish force-pushed the metricsdisplay branch from 94edb8c to f668318 Compare May 7, 2018 19:11

patrickhulce approved these changes May 7, 2018

View reviewed changes

brendankenny reviewed May 7, 2018

View reviewed changes

brendankenny approved these changes May 7, 2018

View reviewed changes

redo getWastedMs and add a test.

f1701e7

paulirish merged commit 1750799 into master May 7, 2018

paulirish deleted the metricsdisplay branch May 7, 2018 21:48

paulirish restored the metricsdisplay branch May 7, 2018 21:48

paulirish deleted the metricsdisplay branch June 15, 2018 23:23

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

report: final metrics display, icons, whitespace polish (GoogleChrome…

c38d3ca

…#5130)

	const wastedMsValues = opportunityAudits.map(audit => this._getWastedMs(audit));
	const maxWaste = Math.max(...wastedMsValues);

report: final metrics display, icons, whitespace polish #5130

report: final metrics display, icons, whitespace polish #5130

Conversation

paulirish commented May 5, 2018 • edited Loading

screenshots

example reports:

paulirish commented May 5, 2018

vinamratasingal-zz commented May 5, 2018

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

paulirish commented May 5, 2018

paulirish commented May 5, 2018

vinamratasingal-zz commented May 5, 2018

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

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

paulirish commented May 7, 2018 via email

paulirish commented May 7, 2018

paulirish commented May 7, 2018

brendankenny 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

paulirish May 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish May 7, 2018 • edited Loading

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

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented May 7, 2018

paulirish commented May 5, 2018 •

edited

Loading

paulirish May 7, 2018 •

edited

Loading

paulirish May 7, 2018 •

edited

Loading