report: display final screenshot prominently #13123
Conversation
paulirish
requested review from
connorjclark
and removed request for
a team
| @@ -255,6 +256,8 @@ export class ReportRenderer { | |||
| )); | |||
| } | |||
|
|
|||
| categoryRenderer.injectFinalScreenshot(categories, report.audits, scoreScale); | |||
There was a problem hiding this comment.
scoreScale is always set... did you mean to set it only if scoreHeader is true?
There was a problem hiding this comment.
in the firstElementChild case, that was sometimes undefined (according to TSC), thus the if statements. but now with your better "always use a wrapper div" solution, we don't have that problem.
we still want to move the scorescale, but only if we have a final-screenshot. so.. that's why its appended first in the scoreheader, but then moved later on.
report/assets/styles.css
Outdated
| @@ -1066,6 +1065,10 @@ | |||
| margin-top: var(--audit-group-padding-vertical); | |||
| } | |||
|
|
|||
| .lh-audit-group--metrics > .lh-audit-group__header { | |||
| visibility: hidden; /* Superfluous, because obviously perf metrics */ | |||
There was a problem hiding this comment.
I don't understand this... if this is not needed, why have?
There was a problem hiding this comment.
The title of "Metrics" is removed in the mock. It's one of the items in the FR Design Notes doc list.
This DOM element is added when when perf renderer calls the generic renderAuditGroup. so.. trying to omit the element would be a bit more awkward.
also its vis hidden (and not display:none) because spacing-wise it's still needed thanks to the PILL. stupid pill.
There was a problem hiding this comment.
gotcha. Could you add some version of that as a comment, the current comment is too opaque :)
|
Did you mean to get rid of the border around the score scale?
->
also text looks different. perhaps a classname was lost in the transition? |
yup. i just followed the mock for that. |
|
nice review thx for the close look. yeah i didnt look at mobile/narrow viewport much and am sure it can use some love. |
|
Co-authored-by: Connor Clark <cjamcl@google.com>
report/renderer/category-renderer.js
Outdated
| * @param {Element} scoreScaleEl | ||
| */ | ||
| injectFinalScreenshot(categoriesEl, audits, scoreScaleEl) { | ||
| // TODO: Use full-page-screenshot instead as that's always gathered, regardless of category filter |
There was a problem hiding this comment.
Can you file an issue and remove this todo?
update: this PR is now blocked by #13229
Adopt the final screenshot display tweaks from latest psi mocks.
Fixes #10016. And is an alternative solution for our classic #9379 (desktop indicator) one as well.
If the final-screenshot audit is available to the renderer.. it'll do this kinda layout to the topmost category (which.. is Performance 99.9% of the time). That includes moving the scorescale.
Otherwise, the layout will be the same as it is today. This means a solo-accessibility run wont look any different.
Mock:
A few screenshots, as our sample reports don't capture all the various cases
(this last case is only possible if the category order were messed up, like by golang). perf category is needed since this is dependent on final-screenshot. )