Repository: GoogleChrome/lighthouse
Pull request #6482 (2018-11-05 23:14:03)

report: rename clump classes; give classes to all audit groups
working on #6395

The PWA category renderer needs to add custom icons to its groups (like the performance renderer does), but to do so without creating each group itself or grossly reaching into things, this PR sets it up by adding a class name to each group based on the group's id in the config (`.lh-audit-group--${groupId}`, modelled after the existing `lh-audit-group--metrics`, `lh-audit-group--opportunities`, etc).

To not collide with anything, the clump names need to move off a mishmash of class names (`lh-failed-audits`, `lh-audit-group--manual`, etc). So we'll just double down on "clump" for those :)

This PR also removes `lh-audit-group--unadorned` since "unadorned" is not a group name and it's somewhat ambiguous what should fall into that class, but that leaves a bit of a mess with `::before` content and margins. I believe I have that all straightened out now. Previously the "unadorned" case took things away from the styling of clumps and important groups, now it's more the default style and clumps and important groups add to it.


Patch:
diff --git a/lighthouse-core/report/html/renderer/category-renderer.js b/lighthouse-core/report/html/renderer/category-renderer.js
index 39efef223..5b4b7b0c2 100644
--- a/lighthouse-core/report/html/renderer/category-renderer.js
+++ b/lighthouse-core/report/html/renderer/category-renderer.js
@@ -46,19 +46,19 @@ class CategoryRenderer {
   get _clumpDisplayInfo() {
     return {
       'failed': {
-        className: 'lh-failed-audits',
+        className: 'lh-clump--failed',
       },
       'manual': {
         title: Util.UIStrings.manualAuditsGroupTitle,
-        className: 'lh-audit-group--manual',
+        className: 'lh-clump--manual',
       },
       'passed': {
         title: Util.UIStrings.passedAuditsGroupTitle,
-        className: 'lh-passed-audits',
+        className: 'lh-clump--passed',
       },
       'not-applicable': {
         title: Util.UIStrings.notApplicableAuditsGroupTitle,
-        className: 'lh-audit-group--not-applicable',
+        className: 'lh-clump--not-applicable',
       },
     };
   }
@@ -255,7 +255,7 @@ class CategoryRenderer {
       for (const auditRef of groupAuditRefs) {
         auditGroupElem.appendChild(this.renderAudit(auditRef, index++));
       }
-      auditGroupElem.classList.add('lh-audit-group--unadorned');
+      auditGroupElem.classList.add(`lh-audit-group--${groupId}`);
       auditElements.push(auditGroupElem);
     }
 
@@ -300,7 +300,7 @@ class CategoryRenderer {
     if (clumpId === 'failed') {
       // Failed audit clump is always expanded and not nested in an lh-audit-group.
       const failedElem = this.renderUnexpandableClump(auditRefs, groupDefinitions);
-      failedElem.classList.add(this._clumpDisplayInfo.failed.className);
+      failedElem.classList.add('lh-clump', this._clumpDisplayInfo.failed.className);
       return failedElem;
     }
 
@@ -312,7 +312,7 @@ class CategoryRenderer {
     const groupDef = {title: clumpInfo.title, description};
     const opts = {expandable, itemCount: auditRefs.length};
     const clumpElem = this.renderAuditGroup(groupDef, opts);
-    clumpElem.classList.add(clumpInfo.className);
+    clumpElem.classList.add('lh-clump', clumpInfo.className);
 
     elements.forEach(elem => clumpElem.appendChild(elem));
 
diff --git a/lighthouse-core/report/html/report-styles.css b/lighthouse-core/report/html/report-styles.css
index 335604b2d..55b1e64b3 100644
--- a/lighthouse-core/report/html/report-styles.css
+++ b/lighthouse-core/report/html/report-styles.css
@@ -579,7 +579,8 @@ details, summary {
 }
 
 .lh-audit-group__header::before {
-  content: '';
+  /* By default, groups don't get an icon */
+  content: none;
   width: calc(var(--subheader-font-size) / 14 * 24);
   height: calc(var(--subheader-font-size) / 14 * 24);
   margin-right: calc(var(--subheader-font-size) / 2);
@@ -589,30 +590,31 @@ details, summary {
   vertical-align: middle;
 }
 
-/* A11y/Seo groups within Passed don't get an icon */
-.lh-audit-group--unadorned .lh-audit-group__header::before {
-  content: none;
-}
-
-
-.lh-audit-group--manual .lh-audit-group__header::before {
+.lh-clump--manual > summary .lh-audit-group__header::before {
+  content: '';
   background-image: var(--search-icon-url);
 }
-.lh-passed-audits .lh-audit-group__header::before {
+.lh-clump--passed > summary .lh-audit-group__header::before {
+  content: '';
   background-image: var(--check-icon-url);
 }
+.lh-clump--not-applicable > summary .lh-audit-group__header::before {
+  content: '';
+  background-image: var(--remove-circle-icon-url);
+}
+
 .lh-audit-group--diagnostics .lh-audit-group__header::before {
+  content: '';
   background-image: var(--content-paste-icon-url);
 }
 .lh-audit-group--opportunities .lh-audit-group__header::before {
+  content: '';
   background-image: var(--photo-filter-icon-url);
 }
 .lh-audit-group--metrics .lh-audit-group__header::before {
+  content: '';
   background-image: var(--av-timer-icon-url);
 }
-.lh-audit-group--not-applicable .lh-audit-group__header::before {
-  background-image: var(--remove-circle-icon-url);
-}
 
 /* Removing too much whitespace */
 .lh-audit-group--metrics {
@@ -637,11 +639,14 @@ details, summary {
 .lh-audit-group__description {
   font-size: var(--body-font-size);
   color: var(--medium-75-gray);
-  margin: var(--lh-audit-group-vpadding) 0;
+  margin: 0 0 var(--lh-audit-group-vpadding);
 }
 
-.lh-audit-group--unadorned .lh-audit-group__description {
-  margin-top: 0;
+.lh-clump > .lh-audit-group__description,
+.lh-audit-group--diagnostics .lh-audit-group__description,
+.lh-audit-group--opportunities .lh-audit-group__description,
+.lh-audit-group--metrics .lh-audit-group__description {
+  margin-top: var(--lh-audit-group-vpadding);
 }
 
 .lh-audit-explanation {


Test patch:
diff --git a/lighthouse-core/test/report/html/renderer/category-renderer-test.js b/lighthouse-core/test/report/html/renderer/category-renderer-test.js
index dfa30610d..ad69de5b0 100644
--- a/lighthouse-core/test/report/html/renderer/category-renderer-test.js
+++ b/lighthouse-core/test/report/html/renderer/category-renderer-test.js
@@ -142,13 +142,13 @@ describe('CategoryRenderer', () => {
   it('renders manual audits if the category contains them', () => {
     const pwaCategory = sampleResults.reportCategories.find(cat => cat.id === 'pwa');
     const categoryDOM = renderer.render(pwaCategory, sampleResults.categoryGroups);
-    assert.ok(categoryDOM.querySelector('.lh-audit-group--manual .lh-audit-group__summary'));
+    assert.ok(categoryDOM.querySelector('.lh-clump--manual .lh-audit-group__summary'));
     assert.equal(categoryDOM.querySelectorAll('.lh-audit--manual').length, 3,
         'score shows informative and dash icon');
 
     assert.ok(pwaCategory.manualDescription);
     const description = categoryDOM
-      .querySelector('.lh-audit-group--manual .lh-audit-group__description').textContent;
+      .querySelector('.lh-clump--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');
@@ -158,19 +158,19 @@ describe('CategoryRenderer', () => {
     const a11yCategory = sampleResults.reportCategories.find(cat => cat.id === 'accessibility');
     const categoryDOM = renderer.render(a11yCategory, sampleResults.categoryGroups);
     assert.ok(categoryDOM.querySelector(
-        '.lh-audit-group--not-applicable .lh-audit-group__summary'));
+        '.lh-clump--not-applicable .lh-audit-group__summary'));
 
     const notApplicableCount = a11yCategory.auditRefs.reduce((sum, audit) =>
         sum += audit.result.scoreDisplayMode === 'not-applicable' ? 1 : 0, 0);
     assert.equal(
-      categoryDOM.querySelectorAll('.lh-audit-group--not-applicable .lh-audit').length,
+      categoryDOM.querySelectorAll('.lh-clump--not-applicable .lh-audit').length,
       notApplicableCount,
       'score shows informative and dash icon'
     );
 
     const bestPracticeCat = sampleResults.reportCategories.find(cat => cat.id === 'best-practices');
     const categoryDOM2 = renderer.render(bestPracticeCat, sampleResults.categoryGroups);
-    assert.ok(!categoryDOM2.querySelector('.lh-audit-group--not-applicable'));
+    assert.ok(!categoryDOM2.querySelector('.lh-clump--not-applicable'));
   });
 
   describe('category with groups', () => {
@@ -216,7 +216,7 @@ describe('CategoryRenderer', () => {
           audit.result.scoreDisplayMode !== 'not-applicable' && audit.result.score === 1);
       const passedAuditTags = new Set(passedAudits.map(audit => audit.group));
 
-      const passedAuditGroups = categoryDOM.querySelectorAll('.lh-passed-audits .lh-audit-group');
+      const passedAuditGroups = categoryDOM.querySelectorAll('.lh-clump--passed .lh-audit-group');
       assert.equal(passedAuditGroups.length, passedAuditTags.size);
     });
 
@@ -229,11 +229,11 @@ describe('CategoryRenderer', () => {
     it('increments the audit index across groups', () => {
       const elem = renderer.render(category, sampleResults.categoryGroups);
 
-      const passedAudits = elem.querySelectorAll('.lh-passed-audits .lh-audit__index');
-      const failedAudits = elem.querySelectorAll('.lh-failed-audits .lh-audit__index');
-      const manualAudits = elem.querySelectorAll('.lh-audit-group--manual .lh-audit__index');
+      const passedAudits = elem.querySelectorAll('.lh-clump--passed .lh-audit__index');
+      const failedAudits = elem.querySelectorAll('.lh-clump--failed .lh-audit__index');
+      const manualAudits = elem.querySelectorAll('.lh-clump--manual .lh-audit__index');
       const notApplicableAudits =
-        elem.querySelectorAll('.lh-audit-group--not-applicable .lh-audit__index');
+        elem.querySelectorAll('.lh-clump--not-applicable .lh-audit__index');
 
       const assertAllTheIndices = (nodeList) => {
         // Must be at least one for a decent test.
@@ -274,15 +274,29 @@ describe('CategoryRenderer', () => {
         assert.ok(ungroupedAudits.includes(auditId), auditId);
       }
     });
+
+    it('gives each group a selectable class', () => {
+      const categoryGroupIds = new Set(category.auditRefs.filter(a => a.group).map(a => a.group));
+      assert.ok(categoryGroupIds.size > 6); // Ensure there's something to test.
+
+      const categoryElem = renderer.render(category, sampleResults.categoryGroups);
+
+      categoryGroupIds.forEach(groupId => {
+        const selector = `.lh-audit-group--${groupId}`;
+        // Could be multiple results (e.g. found in both passed and failed clumps).
+        assert.ok(categoryElem.querySelectorAll(selector).length >= 1,
+          `could not find '${selector}'`);
+      });
+    });
   });
 
-  describe('grouping passed/failed/manual', () => {
+  describe('clumping passed/failed/manual', () => {
     it('separates audits in the DOM', () => {
       const category = sampleResults.reportCategories.find(c => c.id === 'pwa');
       const elem = renderer.render(category, sampleResults.categoryGroups);
-      const passedAudits = elem.querySelectorAll('.lh-passed-audits .lh-audit');
-      const failedAudits = elem.querySelectorAll('.lh-failed-audits .lh-audit');
-      const manualAudits = elem.querySelectorAll('.lh-audit-group--manual .lh-audit');
+      const passedAudits = elem.querySelectorAll('.lh-clump--passed .lh-audit');
+      const failedAudits = elem.querySelectorAll('.lh-clump--failed .lh-audit');
+      const manualAudits = elem.querySelectorAll('.lh-clump--manual .lh-audit');
 
       assert.equal(passedAudits.length, 4);
       assert.equal(failedAudits.length, 8);
@@ -294,13 +308,11 @@ describe('CategoryRenderer', () => {
       const category = JSON.parse(JSON.stringify(origCategory));
       category.auditRefs.forEach(audit => audit.result.score = 0);
       const elem = renderer.render(category, sampleResults.categoryGroups);
-      const passedAudits = elem.querySelectorAll('.lh-passed-audits .lh-audit');
-      const failedAudits = elem.querySelectorAll('.lh-failed-audits .lh-audit');
+      const passedAudits = elem.querySelectorAll('.lh-clump--passed .lh-audit');
+      const failedAudits = elem.querySelectorAll('.lh-clump--failed .lh-audit');
 
       assert.equal(passedAudits.length, 0);
       assert.equal(failedAudits.length, 12);
-
-      assert.equal(elem.querySelector('.lh-passed-audits-summary'), null);
     });
   });
 
diff --git a/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js b/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js
index 86b16c3b3..636eebca2 100644
--- a/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js
+++ b/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js
@@ -147,7 +147,7 @@ describe('PerfCategoryRenderer', () => {
 
   it('renders the passed audits', () => {
     const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
-    const passedSection = categoryDOM.querySelector('.lh-category > .lh-passed-audits');
+    const passedSection = categoryDOM.querySelector('.lh-category > .lh-clump--passed');
 
     const passedAudits = category.auditRefs.filter(audit =>
         audit.group && audit.group !== 'metrics' && Util.showAsPassed(audit.result));