report: DRY up audit & opportunity rendering #5136

paulirish · 2018-05-06T02:05:31Z

Gets rid of a lot of duplicated work, and brings a lovely consistency to the styles.

https://lighthouse-oppoasanaudit.now.sh/

patrickhulce

not 100% sure what this buys us functionality-wise, slightly nervous we'll get fancier with the opportunities and need to fork again, but I'm probably missing the grand vision

patrickhulce · 2018-05-06T06:32:04Z

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


+    // Don't mutate the value
+    displayValue = [...displayValue];


oh heh, I guess we might want to use them more than once huh :) thanks for the cleanup!

wanna just remove the mutations? const template = displayValue[0]; ... of displayValue.slice(1)

add a util-test.js test for mutation?

patrickhulce · 2018-05-06T06:39:36Z

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

   * @return {!Element}
   */
-  renderAudit(audit, index) {
-    const tmpl = this.dom.cloneTemplate('#tmpl-lh-audit', this.templateContext);
+  renderAudit(audit, index, providedTmpl) {


feels like this sorta becomes populateAuditValues instead, is that fair?

essentially yeah.

patrickhulce · 2018-05-06T06:41:53Z

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

+    this.dom.find('.lh-sparkline__bar', element).style.width = sparklineWidthPct;
+    wastedEl.textContent = Util.formatSeconds(summaryInfo.wastedMs, 0.01);
+
+    if (audit.result.displayValue) {


so if we're overriding displayValue anyhow and there's no helpText, we're using renderAudit for

populating index

setting rating class

sticking it inside of details

filling error/debugString

something else I'm missing?

WDYT about extracting those 4 to a method they both share?

i like the sound of that. will do.

patrickhulce · 2018-05-06T06:43:23Z

lighthouse-core/report/html/report-styles.css

 .lh-audit__header:hover {
  background-color: #F8F9FA;
 }


 .lh-audit-group[open] > .lh-audit-group__summary > .lh-toggle-arrow,
 .lh-expandable-details[open] > .lh-expandable-details__summary > .lh-toggle-arrow,
-.lh-expandable-details[open] > .lh-expandable-details__summary > div > .lh-toggle-arrow {
+.lh-expandable-details[open] > .lh-expandable-details__summary > div > .lh-toggle-arrow,
+.lh-expandable-details[open] > .lh-expandable-details__summary > div > div > .lh-toggle-arrow {


the nest grows deeper :( removed in chevron ✨ though?

yah in #5137 this gets cleaned up a lot.

paulirish · 2018-05-06T21:38:20Z

not 100% sure what this buys us functionality-wise, slightly nervous we'll get fancier with the opportunities and need to fork again, but I'm probably missing the grand vision

There's been a lot of bugs and inconsistencies recently between opps and audits, all stemming from their separate implementations:

opps's toggle-arrow wasn't animating
error'd opps werent being handled the same as error'd audits. They needed the "Error!" displaytext + tooltip.
Different margin/paddings all around. around the description. on the left/right side of the details tables.
debugString placement was different

(To be fair, all these are my fault, but all of these made it past review into master, so... they're sneaky)

In what I have here the templates are defined separately, we're just using the same classes for all the same things. And so we improve maintainability and reduce the duplication.
➡️ ➡️ ➡️

net negative what what

patrickhulce · 2018-05-07T02:36:09Z

There's been a lot of bugs and inconsistencies recently between opps and audits, all stemming from their separate implementations

cool cool, yeah sg with your planned changes 👍

brendankenny · 2018-05-07T18:18:49Z

lighthouse-core/audits/first-meaningful-paint.js

@@ -15,8 +15,8 @@ class FirstMeaningfulPaint extends Audit {
  static get meta() {
    return {
      name: 'first-meaningful-paint',
-      description: 'First meaningful paint',
-      helpText: 'First meaningful paint measures when the primary content of a page is visible. ' +
+      description: 'First Meaningful Paint',


is it possible to squash/rebase/something off of #5130? definitely confusing having all of that in here when it's going to merge with it

rebased. much cleaner.

brendankenny · 2018-05-07T18:22:44Z

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

   * @return {!Element}
   */
-  renderAudit(audit, index) {
-    const tmpl = this.dom.cloneTemplate('#tmpl-lh-audit', this.templateContext);
+  renderAudit(audit, index, providedTmpl) {


docstring would be good to explain that you can provide an alternative template, just needs to match #tmpl-lh-audit in structures a, b, and c

brendankenny · 2018-05-07T18:34:06Z

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


+    // Don't mutate the value
+    displayValue = [...displayValue];


add a util-test.js test for mutation?

paulirish · 2018-05-12T00:36:39Z

(this is ready for another look)

brendankenny

this seems like a good direction to go for now. Dedupes error-prone stuff but still easily separable if they need to go their own ways in the future

brendankenny · 2018-05-15T22:18:55Z

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

    const auditEl = this.dom.find('.lh-audit', tmpl);
    auditEl.id = audit.result.id;
+    const displayTextEl = this.dom.find('.lh-audit__display-text', auditEl);


nit: move this into the conditional where it's used?

brendankenny · 2018-05-15T22:24:02Z

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

-    element.classList.add(`lh-load-opportunity--${Util.calculateRating(audit.result.score)}`);
+    const oppTmpl = this.dom.cloneTemplate('#tmpl-lh-opportunity', this.templateContext);
+    const element = this.populateAuditValues(audit, index, oppTmpl);
+    element.classList.add(`lh-audit--${Util.calculateRating(audit.result.score)}`);


looks like this is now already set in populateAuditValues by _setRatingClass()

lol populateAuditValues also has a stray auditEl.classList.add(lh-audit--${audit.result.scoreDisplayMode}); line right before calling _setRatingClass

hah. thanks for catching both of these

brendankenny · 2018-05-15T22:33:11Z

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

-      const detailsElem = this.detailsRenderer.render(details);
-      detailsElem.classList.add('lh-details');
-      element.appendChild(detailsElem);
+    const wastedEl = this.dom.find('.lh-audit__display-text', element);


maybe comment that everything below here is overwriting the default displayValue display provided by populateAuditValues with opportunity-specific stuff?

brendankenny · 2018-05-15T22:38:59Z

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

+      wastedEl.textContent = Util.formatSeconds(summaryInfo.wastedMs, 0.01);
+    }
+
+    if (audit.result.displayValue) {


this seems weird to do if it was an error scoreDisplayMode. Should be nested inside above error conditional?

thanks. didnt need the error conditional after all.

brendankenny · 2018-05-15T22:41:49Z

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

+    return this.populateAuditValues(audit, index, tmpl);
+  }
+
+  /**


still want a docstring here :P

brendankenny

last questions :)

brendankenny · 2018-05-17T21:45:20Z

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

      },
    };

    const fakeCategory = Object.assign({}, category, {auditRefs: [auditWithDebug]});
    const categoryDOM = renderer.render(fakeCategory, sampleResults.categoryGroups);

-    const debugEl = categoryDOM.querySelector('.lh-load-opportunity .lh-debug');
+    const debugEl = categoryDOM.querySelector('.lh-audit--load-opportunity .lh-debug');


maybe call this tooltipEl? (at least I believe that's what it selects now)

brendankenny · 2018-05-17T21:46:18Z

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

@@ -71,7 +81,7 @@ class CategoryRenderer {
      const textEl = this.dom.find('.lh-audit__display-text', auditEl);
      textEl.textContent = 'Error!';
      textEl.classList.add('tooltip-boundary');
-      const tooltip = this.dom.createChildOf(textEl, 'div', 'lh-error-tooltip-content tooltip');
+      const tooltip = this.dom.createChildOf(textEl, 'div', 'tooltip lh-debug');


does it make sense for the tooltip to share the lh-debug class?

brendankenny · 2018-05-17T21:47:23Z

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

-      detailsElem.classList.add('lh-details');
-      element.appendChild(detailsElem);
+    this.dom.find('.lh-sparkline__bar', element).style.width = sparklineWidthPct;
+    displayEl.textContent = Util.formatSeconds(summaryInfo.wastedMs, 0.01);


feels weird to overwrite Error! with this in the error case. Will it always have a summaryInfo if there was a gatherer or audit error?

brendankenny

LGTMMM

paulirish · 2018-05-17T23:08:11Z

I want to thank the Academy, my wife, Daisy the donkey, and big G.

paulirish requested review from brendankenny, ebidel, patrickhulce and vinamratasingal-zz as code owners May 6, 2018 02:05

paulirish force-pushed the oppoAsAnAudit branch from 53701bf to 59de2ab Compare May 6, 2018 04:33

paulirish mentioned this pull request May 6, 2018

report: animated chevrons #5137

Merged

patrickhulce reviewed May 6, 2018

View reviewed changes

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

brendankenny reviewed May 7, 2018

View reviewed changes

paulirish force-pushed the oppoAsAnAudit branch from 59de2ab to b763737 Compare May 7, 2018 19:06

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

paulirish changed the base branch from metricsdisplay to metricsdisplay-oldrebase May 7, 2018 19:21

paulirish mentioned this pull request May 7, 2018

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

Merged

paulirish added the waiting4committer label May 7, 2018

paulirish force-pushed the oppoAsAnAudit branch from b763737 to 107b912 Compare May 8, 2018 03:37

paulirish added the chillin ❄ label May 8, 2018

paulirish changed the base branch from metricsdisplay-oldrebase to master May 8, 2018 03:56

paulirish force-pushed the oppoAsAnAudit branch from 107b912 to 2a2af27 Compare May 12, 2018 00:16

paulirish deployed to PR staging May 12, 2018 00:16 View deployment

paulirish changed the title ~~report: render opportunities as a subclass of audits~~ report: DRY up audit & opportunity rendering May 12, 2018

paulirish removed chillin ❄ waiting4committer labels May 12, 2018

paulirish deployed to PR staging May 12, 2018 00:36 View deployment

paulirish added 3 commits May 14, 2018 11:22

resolve failing tests.

d538df8

feedback

c1359fc

test failure.

7e2e5aa

paulirish force-pushed the oppoAsAnAudit branch from 60919cf to 7e2e5aa Compare May 14, 2018 18:23

paulirish deployed to PR staging May 14, 2018 18:23 View deployment

brendankenny requested changes May 15, 2018

View reviewed changes

paulirish added 2 commits May 15, 2018 17:54

feedback from brendan.

bc2c815

error audits don't have wastedMs

e1f5ba2

paulirish deployed to PR staging May 16, 2018 01:03 View deployment

safety first.

87b782c

paulirish deployed to PR staging May 16, 2018 01:14 View deployment

paulirish added 2 commits May 17, 2018 12:56

Merge branch 'master' into oppoAsAnAudit

c4ec87e

fix typecheck

59b9e49

paulirish deployed to PR staging May 17, 2018 19:59 View deployment

brendankenny reviewed May 17, 2018

View reviewed changes

feedback.

cd48fd0

paulirish deployed to PR staging May 17, 2018 22:49 View deployment

brendankenny approved these changes May 17, 2018

View reviewed changes

paulirish merged commit a99c07b into master May 17, 2018

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

brendankenny mentioned this pull request Jan 4, 2024

report: small renderAudit simplification #15725

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: DRY up audit & opportunity rendering #5136

report: DRY up audit & opportunity rendering #5136

paulirish commented May 6, 2018 •

edited

Loading

patrickhulce left a comment

patrickhulce May 6, 2018

brendankenny May 7, 2018

patrickhulce May 6, 2018

paulirish May 6, 2018

patrickhulce May 6, 2018

paulirish May 6, 2018

patrickhulce May 6, 2018

paulirish May 6, 2018

paulirish commented May 6, 2018

patrickhulce commented May 7, 2018

brendankenny May 7, 2018

paulirish May 7, 2018

brendankenny May 7, 2018

brendankenny May 7, 2018

paulirish commented May 12, 2018

brendankenny left a comment

brendankenny May 15, 2018

brendankenny May 15, 2018

brendankenny May 15, 2018

paulirish May 16, 2018

brendankenny May 15, 2018

brendankenny May 15, 2018

paulirish May 16, 2018

brendankenny May 15, 2018

paulirish May 16, 2018

brendankenny left a comment

brendankenny May 17, 2018

brendankenny May 17, 2018

paulirish May 17, 2018

brendankenny May 17, 2018

brendankenny left a comment

paulirish commented May 17, 2018

report: DRY up audit & opportunity rendering #5136

report: DRY up audit & opportunity rendering #5136

Conversation

paulirish commented May 6, 2018 • edited 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

paulirish commented May 6, 2018

patrickhulce commented May 7, 2018

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 12, 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

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

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 17, 2018

paulirish commented May 6, 2018 •

edited

Loading