report: show metric descriptions by default when errors exist #9054
Conversation
| it('with no errors, hide by default', () => { | ||
| const lhr = JSON.parse(JSON.stringify(sampleResults)); | ||
| const container = render(lhr); | ||
| assert.ok(!container.querySelector('.lh-metrics-toggle__input').checked); |
There was a problem hiding this comment.
these assertions seem questionable..
There was a problem hiding this comment.
as in, I'm very explicitly testing against the if statement. idk seems odd
There was a problem hiding this comment.
yeah it feels like we really just want to assert that the errors are visible, do we do any gCS in these UI tests yet?
There was a problem hiding this comment.
gCS?
I first tried to do something like "are the description elements visible", but I don't think we can do that b/c jsdom doesn't try to surface stuff like that. as in, there is no rendering/css engine.
There was a problem hiding this comment.
I thought styles directly applied to elements still resolved though? I could be wrong I just thought there was no cascading.
There was a problem hiding this comment.
Ah, so I was actually thinking of layout (from reading this). Tried a bit more, but still no dice:
There was a problem hiding this comment.
LOL I forgot we shim getComputedStyles in this test. I thought it was provided by jsdom. anyways, idk how to do this.
There was a problem hiding this comment.
so if we don't overwrite their getComputedStyles, this happens:
weird
| @@ -117,6 +117,13 @@ class ReportUIFeatures { | |||
| this._document.addEventListener('scroll', this._updateStickyHeaderOnScroll); | |||
| window.addEventListener('resize', this._updateStickyHeaderOnScroll); | |||
| } | |||
|
|
|||
| // Show the metric descriptions by default when there is an error. | |||
| if (report.audits.metrics.errorMessage) { | |||
There was a problem hiding this comment.
worth noting that TTI, speed index, and FCPUI will just be undefined and not throw an errorMessage onto metrics so this will only handle it for fcp/fmp/eil/traceOfTab errors
but that seems to be the case we want to auto-expand anyhow, when all of them fail basically?
There was a problem hiding this comment.
My intention was to expand if there are any "Error"s in the metric table. I assumed audits.metrics.errorMessage would be that, but that's probably a bad assumption...
There was a problem hiding this comment.
ah you'll probably want to switch to report.audits['first-contentful-paint'].errorMessage || report.audits.interactive.errorMessage || ... then :/
There was a problem hiding this comment.
figured it out
| @@ -117,6 +117,15 @@ class ReportUIFeatures { | |||
| this._document.addEventListener('scroll', this._updateStickyHeaderOnScroll); | |||
| window.addEventListener('resize', this._updateStickyHeaderOnScroll); | |||
| } | |||
|
|
|||
| // Show the metric descriptions by default when there is an error. | |||
| const hasMetricError = report.categories.performance && report.categories.performance.auditRefs | |||
See #9019 (comment)