report: New audit clump for audits with warnings (#5327) #6989

connorjclark · 2019-01-11T18:14:21Z

connorjclark · 2019-01-11T18:16:02Z

We'll need a new icon. Ideas?
@hwikyounglee

brendankenny

My comments below were based on my assumption going in that this was going to be a new clump for any passed audits that have a warning, but after going through the whole thing I realized you were (maybe?) intending to put any audit with a warning in the new clump. We should figure out which one we want to do before proceeding :)

brendankenny · 2019-01-11T21:00:47Z

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

@@ -264,6 +265,10 @@ class CategoryRenderer {
    const clumpTmpl = this.dom.cloneTemplate('#tmpl-lh-clump', this.templateContext);
    const clumpElement = this.dom.find('.lh-clump', clumpTmpl);

+    if (clumpId === 'warning') {


do we want it to be an open <details> or do we want it to be like failed audits and use renderUnexpandableClump? It seems like it should maybe operate like failed, but I'm not sure how others feel on this point.

renderUnexpandableClump's don't have titles/descriptions so .... let's stick with this approach.
(If it supported titles, I would have said we do that.)

brendankenny · 2019-01-11T21:03:25Z

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

@@ -477,6 +477,8 @@ Util.UIStrings = {
  warningHeader: 'Warnings: ',
  /** The tooltip text on an expandable chevron icon. Clicking the icon expands a section to reveal a list of audit results that was hidden by default. */
  auditGroupExpandTooltip: 'Show audits',
+  /** Section heading shown above a list of audits that contain warnings. */
+  warningAuditsGroupTitle: 'Audits with warnings',


I feel like we should be more specific with something like Passed audits with warnings. And is "with warnings" how we want to phrase it?

wb Passed but contains (or with) warnings? the manual and the n/a titles don't explicitly say "audit".

brendankenny · 2019-01-11T21:05:07Z

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

@@ -477,6 +477,8 @@ Util.UIStrings = {
  warningHeader: 'Warnings: ',
  /** The tooltip text on an expandable chevron icon. Clicking the icon expands a section to reveal a list of audit results that was hidden by default. */
  auditGroupExpandTooltip: 'Show audits',
+  /** Section heading shown above a list of audits that contain warnings. */


Need some context for "warnings" for the translators ("contain warnings" is pretty ambiguous in isolation)...what they are, how they're used sort of thing. See below for examples.

brendankenny · 2019-01-11T21:09:51Z

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

-    if (Util.showAsPassed(auditRef.result)) {
+    if (this._auditHasWarning(auditRef)) {
+      return 'warning';
+    } else if (Util.showAsPassed(auditRef.result)) {


hmm, we should figure out what we want the priority order to be here. Does warning win over failed?

changed to require a passing state for all warnings.

brendankenny · 2019-01-11T21:11:16Z

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

      const manualAudits = elem.querySelectorAll('.lh-clump--manual .lh-audit');

      assert.equal(passedAudits.length, 4);
      assert.equal(failedAudits.length, 8);
+      assert.equal(warningAudits.length, 0);


this test doesn't end up testing much :) WDYT about adding a warning to one of the passed audits?

There are no passing audits with warnings in the sample json, and I'm not sure how to update the source appropriately. fine to just manually set some warnings in the test case?

fine to just manually set some warnings in the test case

perfect

brendankenny · 2019-01-11T21:19:22Z

I think based on reading #5327 the intention was only to make sure passed audits with warnings stay visible instead of being hidden away. Since failed audits are already visible, they aren't an issue. Also if the audit failed for the page it makes sense (to me) that that should take precedence for where it should be located in the report.

@paulirish for what he intended (and maybe to weigh in on the clump or unexpandableClump question)

paulirish

think based on reading #5327 the intention was only to make sure passed audits with warnings stay visible instead of being hidden away. Since failed audits are already visible, they aren't an issue. Also if the audit failed for the page it makes sense (to me) that that should take precedence for where it should be located in the report.

yah any failing audits with warnings are already visible and viewed. We should only put non-failed warning audits in this clump.

paulirish · 2019-01-11T21:33:36Z

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

@@ -264,6 +265,10 @@ class CategoryRenderer {
    const clumpTmpl = this.dom.cloneTemplate('#tmpl-lh-clump', this.templateContext);
    const clumpElement = this.dom.find('.lh-clump', clumpTmpl);

+    if (clumpId === 'warning') {


renderUnexpandableClump's don't have titles/descriptions so .... let's stick with this approach.
(If it supported titles, I would have said we do that.)

brendankenny

wb Passed but contains (or with) warnings? the manual and the n/a titles don't explicitly say "audit".

I'm liking "Passed but contain/with warnings". My only hesitation is that this comes before "Passed audits", which might make that weird.

brendankenny · 2019-01-11T23:11:35Z

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

@@ -382,6 +399,7 @@ class CategoryRenderer {
    /** @type {Map<TopLevelClumpId, Array<LH.ReportResult.AuditRef>>} */
    const clumps = new Map();
    clumps.set('failed', []);
+    clumps.set('warning', []);


should add 'warning' to the list of statuses in the method jsdoc for render() above

brendankenny · 2019-01-11T23:17:59Z

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

@@ -129,6 +129,17 @@ describe('CategoryRenderer', () => {
    assert.ok(warningEl.textContent.includes(auditResult.warnings[1]), '2nd warning provided');
  });

+  it('expands warning audit group', () => {


nit: move this under the clumping describe() since it's really about the clump correctly working

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

brendankenny · 2019-01-11T23:31:03Z

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

+          audit.result.warnings = ['Some warning'];
+          if (++forcedWarningsCounter === 2) break;
+        }
+      }


nit: what about

const categoryClone = JSON.parse(JSON.stringify(category)); // Give the first two passing grades warnings. const passingRefs = categoryClone.auditRefs.filter(ref => ref.result.score === 1); passingRefs[0].result.warnings = ['Some warning']; passingRefs[1].result.warnings = ['Some warning'];

brendankenny · 2019-01-11T23:31:28Z

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

@@ -477,6 +477,8 @@ Util.UIStrings = {
  warningHeader: 'Warnings: ',
  /** The tooltip text on an expandable chevron icon. Clicking the icon expands a section to reveal a list of audit results that was hidden by default. */
  auditGroupExpandTooltip: 'Show audits',
+  /** Section heading shown above a list of passed audits that contain warnings. Audits under this section do no negatively impact the score, but Lighthouse has generated some potentially actionable suggestions that should be reviewed. This section is expanded by default and displays after the failing audits. */


do not negatively

exterkamp

Design feedback: I really think the 1px border on lh-warnings sticks out from the rest of the style. I'd like to hear @hwikyounglee's thoughts on this, but I think using the Lighthouse light orange color (#fff1e5) to fill in the warnings looks much nicer. Thoughts?

Also, should we style the warnings consistently?:

exterkamp · 2019-01-14T18:14:54Z

I think I'm a little confused on the final state of this? Are we doing a Passed Audits with Warnings section?
If no, can we remove all the proto and i18n code for that title? If yes, is this code close to finalized?

brendankenny · 2019-01-14T21:31:12Z

I really think the 1px border on lh-warnings sticks out from the rest of the style.

let's move that into a separate issue. This is the styling of warnings today; this PR is only making any warnings that would have been hidden in Passed audits visible by default.

Are we doing a Passed Audits with Warnings section?

yes, modulo the clump name.

If yes, is this code close to finalized?

yes

connorjclark · 2019-01-14T22:53:55Z

@exterkamp Will you make an issue or a PR for that?

@brendankenny I went with Passed audits but with warnings

I made a warning sign for this clump:

brendankenny

need yarn diff:sample-json

:groundhog_day:

brendankenny · 2019-01-14T23:12:00Z

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

@@ -45,6 +45,7 @@ class CategoryRenderer {
   */
  get _clumpTitles() {
    return {
+      warning: Util.UIStrings.warningAuditsGroupTitle,


it's unfortunate we used "group" for these :/

brendankenny · 2019-01-14T23:12:39Z

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

    } else {
      return 'failed';
    }
  }

  /**
-   * Renders a set of top level sections (clumps), under a status of failed, manual,
-   * passed, or notApplicable. The result ends up something like:
+   * Renders a set of top level sections (clumps), under a status of failed, warnning,


brendankenny · 2019-01-14T23:33:10Z

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

+    });
+
+    it('only passing audits with warnings show in warnings section', () => {
+      const shouldBeFailed = renderer._getClumpIdForAuditRef({


we should avoid touching the internal implementation if we can. We could do something simple like

it('only passing audits with warnings show in warnings section', () => { const failingWarning = 'Failed and warned'; const passingWarning = 'A passing warning'; const category = { id: 'test', title: 'Test', score: 0, auditRefs: [{ id: 'failing', result: { id: 'failing', title: 'Failing with warning', description: '', scoreDisplayMode: 'numeric', score: 0, warnings: [failingWarning], }, }, { id: 'passing', result: { id: 'passing', title: 'Passing with warning', description: '', scoreDisplayMode: 'numeric', score: 1, warnings: [passingWarning], }, }], }; const categoryDOM = renderer.render(category); const shouldBeFailed = categoryDOM.querySelectorAll('.lh-clump--failed .lh-audit'); assert.strictEqual(shouldBeFailed.length, 1); assert.strictEqual(shouldBeFailed[0].id, 'failing'); assert.ok(shouldBeFailed[0].textContent.includes(failingWarning)); const shouldBeWarning = categoryDOM.querySelectorAll('.lh-clump--warning .lh-audit'); assert.strictEqual(shouldBeWarning.length, 1); assert.strictEqual(shouldBeWarning[0].id, 'passing'); assert.ok(shouldBeWarning[0].textContent.includes(passingWarning)); });

(or we could do like the other ones do and just modify an existing category)

brendankenny

just those last little things but otherwise LGTM!

brendankenny · 2019-01-15T01:03:09Z

🎉 🎉

report: New audit clump for audits with warnings (#5327)

bb6b1aa

connorjclark requested review from brendankenny, exterkamp and paulirish as code owners January 11, 2019 18:14

connorjclark mentioned this pull request Jan 11, 2019

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

Closed

brendankenny added the 4.0 label Jan 11, 2019

brendankenny reviewed Jan 11, 2019

View reviewed changes

paulirish reviewed Jan 11, 2019

View reviewed changes

cr changes

d660748

brendankenny reviewed Jan 11, 2019

View reviewed changes

exterkamp reviewed Jan 14, 2019

View reviewed changes

connorjclark added 2 commits January 14, 2019 14:41

edit comments, add test

fb8834b

warning clump icon

4dcd6dc

exterkamp mentioned this pull request Jan 14, 2019

Consistent style for lh-warnings #7011

Closed

brendankenny reviewed Jan 14, 2019

View reviewed changes

brendankenny approved these changes Jan 14, 2019

View reviewed changes

connorjclark added 2 commits January 14, 2019 16:10

cr changes

3737a96

update sample json

3c322af

brendankenny merged commit 534621c into master Jan 15, 2019

brendankenny deleted the issue-5327-expand-passed-audits-group-with-warnings-pass-3 branch January 15, 2019 01:05

paulirish mentioned this pull request Jun 26, 2019

report: reuse generalized clumping for perf category #9288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: New audit clump for audits with warnings (#5327) #6989

report: New audit clump for audits with warnings (#5327) #6989

connorjclark commented Jan 11, 2019

connorjclark commented Jan 11, 2019

brendankenny left a comment

brendankenny Jan 11, 2019

paulirish Jan 11, 2019

brendankenny Jan 11, 2019

connorjclark Jan 11, 2019

brendankenny Jan 11, 2019

brendankenny Jan 11, 2019

connorjclark Jan 11, 2019

brendankenny Jan 11, 2019

connorjclark Jan 11, 2019

brendankenny Jan 11, 2019

brendankenny commented Jan 11, 2019

paulirish left a comment

paulirish Jan 11, 2019

brendankenny left a comment

brendankenny Jan 11, 2019

brendankenny Jan 11, 2019

brendankenny Jan 11, 2019

brendankenny Jan 11, 2019

exterkamp left a comment •

edited

Loading

exterkamp commented Jan 14, 2019

brendankenny commented Jan 14, 2019

connorjclark commented Jan 14, 2019

brendankenny left a comment

brendankenny Jan 14, 2019

brendankenny Jan 14, 2019

brendankenny Jan 14, 2019

brendankenny left a comment

brendankenny commented Jan 15, 2019

report: New audit clump for audits with warnings (#5327) #6989

report: New audit clump for audits with warnings (#5327) #6989

Conversation

connorjclark commented Jan 11, 2019

connorjclark commented Jan 11, 2019

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

Choose a reason for hiding this comment

brendankenny commented Jan 11, 2019

paulirish left a 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

exterkamp left a comment • edited Loading

Choose a reason for hiding this comment

exterkamp commented Jan 14, 2019

brendankenny commented Jan 14, 2019

connorjclark commented Jan 14, 2019

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

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Jan 15, 2019

exterkamp left a comment •

edited

Loading