report: error'd audits get 'Error!' treatment #5077

paulirish · 2018-05-01T00:56:51Z

changes

what the title said
the report's ❌ and ✅ use standard fail/pass colors
populateScore no longer sets title/description. Just handles the score gauge.
removed right column score/icon for informative/manual audits (no more blue '100's)

screenshots

…port.

vinamratasingal-zz · 2018-05-01T01:02:09Z

Hmmm question: why do warnings appear in red? Or is that out of scope for this PR?

Wondering if we should offer similar tooltip visual treatment for warnings + errors?

patrickhulce

woohoo! so excited about this one 😃 🎉 🍰

patrickhulce · 2018-05-01T01:03:18Z

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

-      return element;
+    if (audit.result.error) {
+      element.classList.add(`lh-perf-metric--error`);
+      descriptionEl.innerHTML = '';


is .textContent = '' not sufficient?

patrickhulce · 2018-05-01T01:03:54Z

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

+      valueEl.textContent = 'Error!';
+      valueEl.classList.add('tooltip-boundary');
+      const content = this.dom.createChildOf(valueEl, 'div', 'lh-error-tooltip-content tooltip');
+      content.textContent = audit.result.debugString || 'Report error: no metric information';


isn't this one just for generic audits?

patrickhulce · 2018-05-01T01:07:01Z

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

+    if (audit.result.error) {
+      auditEl.classList.add(`lh-audit--error`);
+      const valueEl = this.dom.find('.lh-score__value', auditEl);
+      valueEl.textContent = 'Error!';


not sure I love the ! on the end, save that sucker for the category score ;)

I thought felt a little less naked with the bang.

but i'm with the errors being bang-less.

patrickhulce · 2018-05-01T01:07:38Z

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

@@ -38,6 +39,9 @@ class Util {
    } else if (score >= RATINGS.AVERAGE.minScore) {
      rating = RATINGS.AVERAGE.label;
    }
+    if (score === null) {


this is a noop until the FIXME? if so save for that PR?

patrickhulce · 2018-05-01T01:08:54Z

lighthouse-core/report/html/report-styles.css

@@ -953,13 +949,14 @@ summary.lh-passed-audits-summary {
  animation: fadeInTooltip 150ms;
  animation-fill-mode: forwards;
  animation-delay: 900ms;
-  min-width: 15em;
+  min-width: 23em;


just felt right?

Yup. In the new audit-error case the tooltip boundary is super narrow so the tooltip was being displayed at its min-width.

Behold!

paulirish · 2018-05-01T01:09:55Z

Hmmm question: why do warnings appear in red?

Good question.

We do have some warning-ish text right now that we show. We'd prefer to not hide this stuff behind a tooltip.

Here's some of the cases:

I'm open to changing their color.

I tried the 'average-color' orange, but the color contrast is too low:

Or is that out of scope for this PR?

yah basically. i think we need a design decision. The followup eng work doesn't really retrace steps so we're good.

vinamratasingal-zz · 2018-05-01T15:29:41Z

Got it. Not needed for this PR, but I do think that changing the warning color would be good to avoid confusion between warning/error. @hwikyounglee can you help suggest a different color?

Otherwise, LGTM from my side.

brendankenny

LGTM

re: orange and red. Not an a11y expert, but the difference in hues isn't very significant. If it's not intended to be the primary way of telling them apart, though (since the original intent was just to have them be the same color but let page structure differentiate them) maybe that's fine

brendankenny · 2018-05-01T18:55:58Z

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

@@ -16,21 +16,24 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
    const tmpl = this.dom.cloneTemplate('#tmpl-lh-perf-metric', this.templateContext);
    const element = this.dom.find('.lh-perf-metric', tmpl);
    element.id = audit.result.name;
+    // FIXME(paulirish): currently this sets a 'lh-perf-metric--undefined' class on error'd audits
    element.classList.add(`lh-perf-metric--${Util.calculateRating(audit.result.score)}`);


just add a conditional on const rating = Util.calculateRating(audit.result.score); if (rating !== undefined) {element.classList.add(...);}? Since lh-perf-metric--error is added below that seems like it's fine (unless you have more specific future plans)

sorry. the undefined was from back when i was fixing score === null, too. mixing up my branches. sry.

Right now scores are 0 in the case of error's. :(
I split that into a second PR because it opens a big can of worms.

brendankenny · 2018-05-01T18:58:01Z

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

+    if (audit.result.error) {
+      element.classList.add(`lh-perf-metric--error`);
+      descriptionEl.textContent = '';
+      valueEl.textContent = 'Error';


ha, well I actually liked the bangful message. Agree that Error just sitting on its own looks incomplete or like the string accidentally leaked into the html there rather than intentionally placed :)

if it were small caps'd and had a hover state ala material design would you feel differently? :)

ha, I think it's making it feel deliberate rather than any particular way of doing that :) Right now it kind of looks like we had a error string of 'Error\nsomething something' and the rest of the string was cut off by the template or overflow hidden or something

¡Error!

this 💯

brendankenny · 2018-05-01T19:01:12Z

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

+      const valueEl = this.dom.find('.lh-score__value', auditEl);
+      valueEl.textContent = 'Error';
+      valueEl.classList.add('tooltip-boundary');
+      const content = this.dom.createChildOf(valueEl, 'div', 'lh-error-tooltip-content tooltip');


bikeshed: if it's a tooltip, name the variable tooltip?

brendankenny · 2018-05-01T19:01:31Z

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

+      element.classList.add(`lh-perf-metric--error`);
+      descriptionEl.textContent = '';
+      valueEl.textContent = 'Error';
+      const content = this.dom.createChildOf(descriptionEl, 'span', 'lh-error-tooltip-content');


brendankenny · 2018-05-01T19:02:47Z

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

    const scoreOutOf100 = Math.round(score * 100);
    const valueEl = this.dom.find('.lh-score__value', element);
    valueEl.textContent = Util.formatNumber(scoreOutOf100);
    valueEl.classList.add(`lh-score__value--${Util.calculateRating(score)}`,
        `lh-score__value--${scoreDisplayMode}`);

-    this.dom.find('.lh-audit__title, .lh-category-header__title', element).appendChild(


patrickhulce

LGTM!

if you're both on the bang train I can live with Error!, I'll just take it as incentive to eliminate as many of our errors as possible 😆

paulirish · 2018-05-03T03:59:22Z

ready to land when green.

paulirish added 5 commits April 30, 2018 15:59

Handle auditErrors

6e0f1b3

simplify populateScore

440c907

report X and check use standard fail/pass colors

fd3dd6f

remove right column score/icon for informative/manual audits

e8964df

report: non-perf error'd audits get same 'Error!' treatment in the re…

12f89de

…port.

paulirish requested a review from vinamratasingal-zz as a code owner May 1, 2018 00:56

paulirish requested a review from patrickhulce May 1, 2018 00:57

patrickhulce suggested changes May 1, 2018

View reviewed changes

feedback.

7be613e

vinamratasingal-zz approved these changes May 1, 2018

View reviewed changes

brendankenny approved these changes May 1, 2018

View reviewed changes

patrickhulce approved these changes May 1, 2018

View reviewed changes

bckenny feedback

df15794

fix closure compilation.

55dc8f8

paulirish merged commit 734e09b into master May 3, 2018

paulirish deleted the auditerror branch May 3, 2018 18:42

paulirish mentioned this pull request May 4, 2018

[meta] 2018 I/O Burndown #5008

Closed

82 tasks

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

report: error'd audits get 'Error!' treatment (GoogleChrome#5077)

d7d2901

patrickhulce mentioned this pull request Jun 24, 2019

report: show fail icon for errors #9272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: error'd audits get 'Error!' treatment #5077

report: error'd audits get 'Error!' treatment #5077

paulirish commented May 1, 2018

vinamratasingal-zz commented May 1, 2018

patrickhulce left a comment

patrickhulce May 1, 2018

patrickhulce May 1, 2018

patrickhulce May 1, 2018

paulirish May 1, 2018

patrickhulce May 1, 2018

patrickhulce May 1, 2018

paulirish May 1, 2018

paulirish commented May 1, 2018

vinamratasingal-zz commented May 1, 2018

brendankenny left a comment

brendankenny May 1, 2018

paulirish May 3, 2018 •

edited

Loading

brendankenny May 1, 2018

patrickhulce May 1, 2018

brendankenny May 1, 2018

brendankenny May 1, 2018

patrickhulce May 1, 2018

brendankenny May 1, 2018

brendankenny May 1, 2018

brendankenny May 1, 2018

patrickhulce left a comment

paulirish commented May 3, 2018

report: error'd audits get 'Error!' treatment #5077

report: error'd audits get 'Error!' treatment #5077

Conversation

paulirish commented May 1, 2018

changes

screenshots

vinamratasingal-zz commented May 1, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented May 1, 2018

vinamratasingal-zz commented May 1, 2018

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish May 3, 2018 • edited Loading

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

patrickhulce left a comment

Choose a reason for hiding this comment

paulirish commented May 3, 2018

paulirish May 3, 2018 •

edited

Loading