report: Expand by default passed audit groups containing warnings #6279

connorjclark · 2018-10-15T18:41:30Z

Summary
Expands by default passed audit groups, if it contains a warning.

Related Issues/PRs
Fixes #5327

exterkamp

LGTM nice changes 🎉

exterkamp · 2018-10-15T19:42:39Z

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

+    const isExpanded = passedAuditGroupEl.hasAttribute('open');
+    assert.ok(isExpanded, 'Passed audit group with warning should be expanded by default');
+
+    category.auditRefs = prevAuditRefs;


What is this line doing? Cleaning up auditRefs? Can this make a deepCopy of category instead so it doesn't modify it for each test (like 'doesnt create a passed section if there were 0 passed')? e.g.

const origCategory = sampleResults.reportCategories.find(c => c.id === 'pwa'); const category = JSON.parse(JSON.stringify(origCategory));

I followed a pattern I saw in the handles markdown in category descriptions a category test case. Since sampleResults is being modified, and is initialized once and used in all the tests, I needed to restore the state of the auditRefs property for the given category.

I see it is also done in the way you suggested here: doesnt create a passed section if there were 0 passed. So there's two different methods for avoiding unwanted state change. If one is preferred, I can change the other instance to match.

patrickhulce

Nice job @hoten! Ace first contribution!

patrickhulce · 2018-10-15T19:44:52Z

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

   * @return {Element}
   */
-  renderPassedAuditsSection(elements) {
+  renderPassedAuditsSection(elements, expandByDefault) {


We've learned over time that every boolean we add to a function method, we regret eventually :)

let's do an object instead const {expandByDefault} = displayOptions

Ah, ok, thanks for the insight.

patrickhulce · 2018-10-15T19:46:14Z

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

@@ -316,8 +327,10 @@ class CategoryRenderer {
    const manualAudits = auditRefs.filter(audit => audit.result.scoreDisplayMode === 'manual');
    const nonManualAudits = auditRefs.filter(audit => !manualAudits.includes(audit));

-    /** @type {Object<string, {passed: Array<LH.ReportResult.AuditRef>, failed: Array<LH.ReportResult.AuditRef>, notApplicable: Array<LH.ReportResult.AuditRef>}>} */
+    /** @type {Object<string, AuditsByState>} */
    const auditsGroupedByGroup = {};


ByState is very nice, let's rename this much less nice variable name I think I'm responsible for to auditsKeyedByState too :D

)

connorjclark · 2018-10-23T16:30:17Z

@paulirish is this good to go?

brendankenny

can you share a screenshot? This looks like it opens up the whole passed group, while the intention of the issue seems to have just been to make the warnings pop up to the top level? @paulirish since he wrote the issue :)

brendankenny · 2018-10-23T16:39:01Z

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

-            Util.showAsPassed(audit.result))
-        .map((audit, i) => this.renderAudit(audit, i));
+            Util.showAsPassed(audit.result));
+    const passedElements = passedAudits.map((audit, i) => this.renderAudit(audit, i));


brendankenny · 2018-10-23T16:44:08Z

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

-    /** @type {Object<string, {passed: Array<LH.ReportResult.AuditRef>, failed: Array<LH.ReportResult.AuditRef>, notApplicable: Array<LH.ReportResult.AuditRef>}>} */
-    const auditsGroupedByGroup = {};
+    /** @type {Object<string, AuditsByState>} */
+    const auditsKeyedByState = {};


we do lose a little bit of info by the rename (and it sounds like the keys will be states, when really it's the keys of the values of the object). Maybe passStatesByGroupId? scoreStatesByGroupId? Or something better

brendankenny · 2018-10-23T16:49:23Z

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

+   * @return {boolean}
+   */
+  _auditsHaveWarnings(audits) {
+    return audits.some(group => Boolean(group.result.warnings && group.result.warnings.length));


s/group/audit?

Hmm. Wonder how my brain did that.

brendankenny · 2018-10-23T16:51:22Z

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

@@ -372,6 +387,7 @@ class CategoryRenderer {
        groups.passed.forEach((item, i) => auditGroupElem.appendChild(this.renderAudit(item, i)));
        auditGroupElem.classList.add('lh-audit-group--unadorned');
        passedElements.push(auditGroupElem);
+        if (!expandPassedAudits) expandPassedAudits = this._auditsHaveWarnings(groups.passed);


hmm, why is this dependent on previous states of expandPassedAudits? That's not immediately clear

oh I see, because we collect passed elements, not passed audits. Some other ideas (take or leave them):

Maybe a comment (// track if any group has passed audits with warnings or whatever) and expandPassedAudits = expandPassedAudits || this._auditsHaveWarnings(groups.passed); to make it more explicit?

is it worth pulling out the check to just a loop over the passed audits? Or do it up in the nonManualAudits.forEach instead?

OR renderPassedAuditsSection could handle this, looking for the warning DOM element on any of the passed in elements and opening up the group if found

iterating on point 1 - 2:

// check if any group has passed audits with warnings let expandPassedAudits = false; for (const group of [auditsUngrouped, ...Object.values(passStatesByGroupId)]) { if (expandPassedAudits = this._auditsHaveWarnings(group.passed)) { break; } }

I don't like the last option, b/c it's querying the DOM elements for state that is more readily available elsewhere.

connorjclark · 2018-10-23T20:32:28Z

Initial view when there is a warning:

@brendankenny Right ... seems the intention as laid out in the ticket differs from what I understood as the task. @paulirish is the above what you expected?

paulirish · 2018-10-23T23:15:18Z

Admittedly this is different than what the ticket described, but I kind of like this approach. The other UX had some awkwardness about it (repeated audit title, so many bullets).

@hwikyounglee any feelings on this? I'd be fine going with what we have here, but I'm cool if you'd prefer a separate block as #5327 described.

hwikyounglee · 2018-10-23T23:49:55Z

I like the aspect of attaching warnings to each audit.
What's bugging me at the moment are 1) 'expanding the passed audits (could be a long list)' when there's a warning, 2) the row shows the green checkmark (passing) but it has a warning (is it truly passing or not)
For audits with warnings, would it make sense to group them in a separate bucket (e.g. "Audits with warning") rather than listing them under 'Passed audits'?

connorjclark · 2018-10-24T03:04:06Z

For audits with warnings, would it make sense to group them in a separate bucket (e.g. "Audits with warning") rather than listing them under 'Passed audits'?

I like this approach.

paulirish · 2019-01-08T18:54:30Z

Let's do that. #6279 (comment)

connorjclark · 2019-01-09T22:56:15Z

Abandoning for #6974.

connorjclark requested a review from paulirish as a code owner October 15, 2018 18:41

exterkamp reviewed Oct 15, 2018

View reviewed changes

patrickhulce reviewed Oct 15, 2018

View reviewed changes

report: Expand by default passed audit groups containing warnings (#5327

647ae56

)

connorjclark force-pushed the issue-5327-expand-passed-audits-group-with-warnings branch from 9299422 to 647ae56 Compare October 15, 2018 21:13

brendankenny reviewed Oct 23, 2018

View reviewed changes

pr changes

0cf04f1

connorjclark added the needs-discussion label Nov 20, 2018

connorjclark mentioned this pull request Jan 7, 2019

report: expand groups within Passed Audits #6930

Merged

connorjclark closed this Jan 9, 2019

connorjclark removed the needs-discussion label Jan 9, 2019

connorjclark deleted the issue-5327-expand-passed-audits-group-with-warnings branch July 11, 2019 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: Expand by default passed audit groups containing warnings #6279

report: Expand by default passed audit groups containing warnings #6279

connorjclark commented Oct 15, 2018

exterkamp left a comment

exterkamp Oct 15, 2018

connorjclark Oct 15, 2018

patrickhulce left a comment

patrickhulce Oct 15, 2018

connorjclark Oct 15, 2018

patrickhulce Oct 15, 2018

connorjclark commented Oct 23, 2018

brendankenny left a comment

brendankenny Oct 23, 2018

brendankenny Oct 23, 2018

brendankenny Oct 23, 2018

connorjclark Oct 23, 2018

brendankenny Oct 23, 2018

brendankenny Oct 23, 2018

connorjclark Oct 23, 2018

connorjclark commented Oct 23, 2018 •

edited

Loading

paulirish commented Oct 23, 2018

hwikyounglee commented Oct 23, 2018

connorjclark commented Oct 24, 2018

paulirish commented Jan 8, 2019

connorjclark commented Jan 9, 2019

report: Expand by default passed audit groups containing warnings #6279

report: Expand by default passed audit groups containing warnings #6279

Conversation

connorjclark commented Oct 15, 2018

exterkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

connorjclark commented Oct 23, 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

connorjclark commented Oct 23, 2018 • edited Loading

paulirish commented Oct 23, 2018

hwikyounglee commented Oct 23, 2018

connorjclark commented Oct 24, 2018

paulirish commented Jan 8, 2019

connorjclark commented Jan 9, 2019

connorjclark commented Oct 23, 2018 •

edited

Loading