report: refactor rendering of top-level failed/passing/etc sections #6460

brendankenny · 2018-11-02T22:44:17Z

This makes the process of grouping under failed/passing/manual/notApplicable and then again under audit groups a lot easier to understand. It also sets the stage for making a new pwa-category-renderer.js (where they aren't split into failed/passing/etc) much easier.

Before the renderer was mixing up levels of failed/passing/notApplicable and audit groups, putting those states inside groups, then ending up rendering the opposite way (groups inside failed/passing/notApplicable). Manual audits were handled separately, even though it's just another of these states.

Instead, this new code sorts by state, then renders groups in each of them. The code is just a little shorter (longer with new tests), but it's hopefully easier to follow.

Importantly, this makes for an easy interface for renderTopLevelSection (which performance-category-renderer now uses) and renderUnexpandableTopLevelSection (which the pwa one will use).

It also lets us get rid of _getTotalAuditsLength and allows easy numbering across groups now.

The numbering is the only thing that has changed from master (so no changed tests, only new ones). edit: also (collapsed) passed audits in the performance section are now grouped like they are in other categories

brendankenny · 2018-11-02T22:44:54Z

also, we can pick a different name than "top-level section". I couldn't think of one (and overloading "group" seems like a mistake in retrospect :)

brendankenny · 2018-11-02T22:46:31Z

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

+      .querySelector('.lh-audit-group--manual .lh-audit-group__description').textContent;
+    // may need to be adjusted if description includes a link at the beginning
+    assert.ok(description.startsWith(pwaCategory.manualDescription.substring(0, 20)),
+        'no manual description');


we weren't asserting that there was a manual description in the report before

paulirish

This is so great. way cleaner.

paulirish · 2018-11-02T22:56:24Z

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

   */
-  _getTotalAuditsLength(elements) {


so happy you managed to nuke this. 🙌

paulirish · 2018-11-02T23:01:25Z

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

-      auditGroupElem.appendChild(this.renderAudit(audit, i));
-    });
-    return auditGroupElem;
+  renderTopLevelSection(sectionId, auditRefs, groupDefinitions, description) {


renderCategorySection ?

and maybe a little ascii diagram of what accessibility section looks like wrt groups & clumps.

done and done

paulirish · 2018-11-02T23:10:38Z

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

-      element.appendChild(manualEl);
-    }
+    // Render each section.
+    for (const [sectionIdStr, sectionRefs] of Object.entries(sections)) {


We're relying on order of Object.entries(), right? is that safe?

We're relying on order of Object.entries(), right? is that safe?

well, there's safe and then there's safe :)

switched to a Map

paulirish · 2018-11-02T23:25:30Z

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

+    const expandable = true;
+    const elements = this._renderGroupedAudits(auditRefs, groupDefinitions, {expandable});
+
+    const sectionInfo = {


move this outside?

move this outside?

done

brendankenny · 2018-11-03T01:24:18Z

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

+  /**
+   * Display info per top-level clump. Define on class to avoid race with Util init.
+   */
+  get _clumpInfo() {


kind of annoyingly this has to be defined on the class so that Util is available (not a problem in regular use, but Util isn't globally defined yet when this file is required in tests)

this feels like a name that a certain someone would complain conveys no information 😉

_clumpDisplayClasses
_clumpDisplayAttributes
_clumpDisplayInfo at least?

this feels like a name that a certain someone would complain conveys no information

lulz I'm always in favor of more descriptive names. Switching to _clumpDisplayInfo though I can switch it more :)

paulirish

so great. love it.

patrickhulce

looks great!

are we really going to go with "clump" though? 😆 #smooshgatev2

patrickhulce · 2018-11-03T03:31:23Z

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

+  /**
+   * Display info per top-level clump. Define on class to avoid race with Util init.
+   */
+  get _clumpInfo() {


this feels like a name that a certain someone would complain conveys no information 😉

_clumpDisplayClasses
_clumpDisplayAttributes
_clumpDisplayInfo at least?

patrickhulce · 2018-11-03T03:31:51Z

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

+    const grouped = new Map();
+
+    // Add audits without a group first so they will appear first.
+    const notAGroup = 'NotAGroup';


patrickhulce · 2018-11-03T03:34:09Z

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

+   * @param {LH.ReportResult.AuditRef} auditRef
+   * @return {TopLevelClumpId}
+   */
+  _getClumpIdForAuditRef(auditRef) {


love this fn!

patrickhulce · 2018-11-03T03:36:37Z

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

-      auditGroupElem.appendChild(this.renderAudit(audit, i));
-    });
-    return auditGroupElem;
+  renderClump(clumpId, auditRefs, groupDefinitions, description) {


nit: this feels a bit many arguments to remember what all the args are for in order, time for an object?

time for an object?

done

brendankenny · 2018-11-03T18:53:31Z

are we really going to go with "clump" though?

haha, well, @paulirish really didn't like my suggestions, and we couldn't keep calling them groups because that was confusing with the other (config) groups... So we looked at a thesaurus and that one was sufficiently funny.

Maybe a name will more naturally come out later.

report: refactor rendering of top-level failed/passing/etc sections

fc6106d

brendankenny requested a review from paulirish as a code owner November 2, 2018 22:44

brendankenny commented Nov 2, 2018

View reviewed changes

paulirish reviewed Nov 2, 2018

View reviewed changes

clumped

6f065d0

brendankenny commented Nov 3, 2018

View reviewed changes

paulirish approved these changes Nov 3, 2018

View reviewed changes

patrickhulce approved these changes Nov 3, 2018

View reviewed changes

feedback

1f24c50

brendankenny merged commit 648a2fd into master Nov 4, 2018

brendankenny deleted the catren branch November 4, 2018 01:59

brendankenny mentioned this pull request Nov 6, 2018

PWA category refactor tracking issue #6395

Closed

31 tasks

exterkamp mentioned this pull request Nov 15, 2018

Report Renderer: Performance section Passed Audits #6568

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: refactor rendering of top-level failed/passing/etc sections #6460

report: refactor rendering of top-level failed/passing/etc sections #6460

brendankenny commented Nov 2, 2018 •

edited

Loading

brendankenny commented Nov 2, 2018

brendankenny Nov 2, 2018 •

edited

Loading

paulirish left a comment

paulirish Nov 2, 2018

paulirish Nov 2, 2018

paulirish Nov 2, 2018

paulirish Nov 2, 2018

brendankenny Nov 3, 2018

paulirish Nov 2, 2018

brendankenny Nov 3, 2018

paulirish Nov 2, 2018

brendankenny Nov 3, 2018

brendankenny Nov 3, 2018

patrickhulce Nov 3, 2018

brendankenny Nov 3, 2018

paulirish left a comment

patrickhulce left a comment

patrickhulce Nov 3, 2018

patrickhulce Nov 3, 2018

patrickhulce Nov 3, 2018

patrickhulce Nov 3, 2018

brendankenny Nov 3, 2018

brendankenny commented Nov 3, 2018

report: refactor rendering of top-level failed/passing/etc sections #6460

report: refactor rendering of top-level failed/passing/etc sections #6460

Conversation

brendankenny commented Nov 2, 2018 • edited Loading

brendankenny commented Nov 2, 2018

brendankenny Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

paulirish 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

paulirish left a comment

Choose a reason for hiding this comment

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

brendankenny commented Nov 3, 2018

brendankenny commented Nov 2, 2018 •

edited

Loading

brendankenny Nov 2, 2018 •

edited

Loading