report: expand groups within Passed Audits #6930

paulirish · 2019-01-04T20:35:05Z

This was the UX conclusion when PSI noticed the groupings in our new renderer.

brendankenny

I'd prefer this, but I believe the conclusion was that there would be no groups under passed.

patrickhulce

so is the end result that groups are not collapsible anymore? they are always expanded?

patrickhulce · 2019-01-04T20:42:39Z

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

+      // Expanded by default!
+      // 1. The 'failed' clump has all groups expanded.
+      // 2. If a clump is collapsed (passed, n/a), we want the groups within to already be expanded
+      const auditGroupElem = this.renderAuditGroup(groupDef, {expandable: false});


this is a little confusing, it's probably because I don't remember what our opts actually do at this point 😆 so expandable means isExpansionToggleable and it's expanded by default?

agree it's a funny name. expandable means collapsedAndCanBeExpanded

paulirish · 2019-01-04T20:45:00Z

I'd prefer this, but I believe the conclusion was that there would be no groups under passed.

Hmm I dont remember that. but Ok. :)

Here's a GIF showing (1) current master - two collapsed groups under Passed. then (2) this PR - two expanded groups under Passed. The gif doesn't show (C) current PSI - groups are intermingled under Passed.

Brendan thought the conclusion was option C, but I forget, tbh.
@kusler wanna make a call?

brendankenny · 2019-01-04T20:52:13Z

yes, I remember it clearly because I was sad to lose the headings under Passed (I believe the argument was that you don't really need them at that point, since they're just a list of passed audits), and I had just finished my sweet ascii art and didn't want to change it :)

egsweeny · 2019-01-04T22:29:39Z

+1 to the idea of getting rid of both the description text and headers within passed audits/not applicable (i.e. how PSI currently handles it). Simple = good.

The only addition I'd like to make is some link that is presented to the user upon expansion of the passed audit/not applicable categories that sends them to a canonical list of the audits with their respective categories, so that on the off chance that a user wants to map a passed audit to its category it is easy for them to do so. WDYT?

paulirish · 2019-01-05T23:22:04Z

Yah now that I've looked at it more, I support the idea of just dropping group titles/descriptions within these.

its extra text that we don't really need for low-priority content.

paulirish · 2019-01-05T23:22:19Z

I'll update the PR with those changes.

connorjclark · 2019-01-07T19:57:49Z

should #6279 be closed?

paulirish · 2019-01-10T20:28:18Z

There's no grouping within Passed, Not Applicable, Manual any longer.

https://abject-partner.surge.sh/example.html

brendankenny · 2019-01-11T00:08:38Z

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

@@ -272,7 +274,7 @@ class CategoryRenderer {
   */
  renderUnexpandableClump(auditRefs, groupDefinitions) {


weren't you going to rename?

weren't you going to rename?

oh, maybe because pwa-category-renderer.js also uses it?

brendankenny · 2019-01-11T00:10:52Z

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

@@ -291,7 +293,8 @@ class CategoryRenderer {
   *      ├── audit 5
   *      └── audit 6
   * clump (e.g. 'manual')
-   *   ├── …
+   *   ├── audit 1
+   *   ├── audit 2
   *   ⋮


maybe

/** * Renders a clump (a top level report section), under a status of failed, manual, * passed, or notApplicable. The result ends up something like: * * failed clump * ├── audit 1 (w/o group) * ├── audit 2 (w/o group) * ├── audit group * | ├── audit 3 * | └── audit 4 * └── audit group * ├── audit 5 * └── audit 6 * other clump (e.g. 'manual') * ├── audit 1 * ├── audit 2 * ├── … * ⋮

brendankenny · 2019-01-11T00:11:42Z

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

@@ -305,17 +308,15 @@ class CategoryRenderer {
      return failedElem;
    }

-    const expandable = true;
-    const elements = this._renderGroupedAudits(auditRefs, groupDefinitions, {expandable});
-
    const clumpInfo = this._clumpDisplayInfo[clumpId];
    // TODO: renderAuditGroup shouldn't be used to render a clump (since it *contains* audit groups).


this TODO makes no sense now. Maybe // TODO: renderAuditGroup shouldn't be used to render a clump since it's a clump, not a group.

brendankenny · 2019-01-11T00:24:17Z

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

    clumpElem.classList.add('lh-clump', clumpInfo.className);

-    elements.forEach(elem => clumpElem.appendChild(elem));
+    // For all non-failed clumps, we don't group


This might not make much sense outside the context of this PR. Just // Add all audit results to the clump. (or add an intermediate const auditElems = auditRefs.map(...) and can probably avoid the comment altogether)

brendankenny · 2019-01-11T02:16:46Z

proposal on top of this in #6982

brendankenny

LGTMMM

report: expand groups within Passed Audits

4a6fcc7

paulirish requested a review from brendankenny January 4, 2019 20:35

brendankenny reviewed Jan 4, 2019

View reviewed changes

patrickhulce reviewed Jan 4, 2019

View reviewed changes

paulirish added the 4.0 label Jan 5, 2019

brendankenny mentioned this pull request Jan 9, 2019

report: New audit group clump for audits with warnings #6974

Closed

paulirish added 3 commits January 10, 2019 12:06

Don't group within non-failed clumps.

238a533

fixup tests.

83f5731

Merge branch 'master' into expandpassed

6cb7222

brendankenny requested changes Jan 11, 2019

View reviewed changes

brendankenny mentioned this pull request Jan 11, 2019

report: clean up more clump/group/expandable crossover noise #6982

Merged

lint

f567ad2

brendankenny approved these changes Jan 11, 2019

View reviewed changes

paulirish merged commit bc23383 into master Jan 11, 2019

paulirish deleted the expandpassed branch January 11, 2019 02:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: expand groups within Passed Audits #6930

report: expand groups within Passed Audits #6930

paulirish commented Jan 4, 2019

brendankenny left a comment •

edited

Loading

patrickhulce left a comment

patrickhulce Jan 4, 2019

paulirish Jan 4, 2019

paulirish commented Jan 4, 2019

brendankenny commented Jan 4, 2019

egsweeny commented Jan 4, 2019

paulirish commented Jan 5, 2019

paulirish commented Jan 5, 2019

connorjclark commented Jan 7, 2019

paulirish commented Jan 10, 2019

brendankenny Jan 11, 2019

brendankenny Jan 11, 2019

brendankenny Jan 11, 2019

brendankenny Jan 11, 2019

brendankenny Jan 11, 2019

brendankenny commented Jan 11, 2019

brendankenny left a comment

		@@ -272,7 +274,7 @@ class CategoryRenderer {
		*/
		renderUnexpandableClump(auditRefs, groupDefinitions) {

report: expand groups within Passed Audits #6930

report: expand groups within Passed Audits #6930

Conversation

paulirish commented Jan 4, 2019

brendankenny left a comment • edited Loading

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce Jan 4, 2019

Choose a reason for hiding this comment

paulirish Jan 4, 2019

Choose a reason for hiding this comment

paulirish commented Jan 4, 2019

brendankenny commented Jan 4, 2019

egsweeny commented Jan 4, 2019

paulirish commented Jan 5, 2019

paulirish commented Jan 5, 2019

connorjclark commented Jan 7, 2019

paulirish commented Jan 10, 2019

brendankenny Jan 11, 2019

Choose a reason for hiding this comment

brendankenny Jan 11, 2019

Choose a reason for hiding this comment

brendankenny Jan 11, 2019

Choose a reason for hiding this comment

brendankenny Jan 11, 2019

Choose a reason for hiding this comment

brendankenny Jan 11, 2019

Choose a reason for hiding this comment

brendankenny commented Jan 11, 2019

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny left a comment •

edited

Loading