report: updates for devtools roll #5326
Conversation
paulirish
requested review from
brendankenny and
vinamratasingal-zz
as code owners
There was a problem hiding this comment.
Most notable change: scores-header was extracted from header so that devtools can reuse it in its very short header.
the if (this._dom.document().querySelector('.lh-devtools')) checks could instead be done with the subclassing we do on the devtools side, but it makes it a lot easier to see a devtools-like report without bringing in those files. https://gist.github.com/paulirish/3d813ed5867412ab55517ba77eb5f93a#file-generate_report-js-L22-L26
i can begrudingly remove it and move to subclassing instead.
| @@ -124,10 +139,18 @@ class ReportRenderer { | |||
| * @return {DocumentFragment} | |||
| */ | |||
| _renderReport(report) { | |||
| const headerStickyContainer = this._dom.createElement('div', 'lh-header-sticky'); | |||
| headerStickyContainer.appendChild(this._renderReportHeader(report)); | |||
| let headerStickyContainer; | |||
There was a problem hiding this comment.
should we rename this now that it's not really the sticky container?
| @@ -71,6 +71,9 @@ class ReportUIFeatures { | |||
| * @param {ReportJSON} report | |||
| */ | |||
| initFeatures(report) { | |||
| // Quit if it's a devtools report. | |||
| if (this._dom.document().querySelector('.lh-devtools')) return; | |||
There was a problem hiding this comment.
WDYT about a universal isDevTools or something at the beginning rather than check this in multiple places?
| @@ -66,6 +66,7 @@ | |||
| --lh-audit-group-vpadding: 8px; | |||
| --lh-section-vpadding: 12px; | |||
| --chevron-size: 12px; | |||
| --bytes-col-width: 3px; | |||
There was a problem hiding this comment.
this is super confusing to look at on it's own, 0px didn't do the trick right? maybe comment to explain :)
| --audit-group-indent: 16px; | ||
| --audit-indent: 16px; | ||
| --expandable-indent: 16px; | ||
| --bytes-col-width: 3px; |
| width: var(--bytes-col-width); | ||
| } | ||
| th.lh-table-column--bytes { | ||
| white-space: normal!important; |
There was a problem hiding this comment.
are we overwriting anything anymore? do we still need to do this?
| /* Keep bytes columns narrow if they follow urls */ | ||
| .lh-table-column--url + th.lh-table-column--bytes, | ||
| .lh-table-column--url + .lh-table-column--bytes + th.lh-table-column--bytes { | ||
| min-width: var(--bytes-col-width); |
There was a problem hiding this comment.
didn't we conclude we could get away with just width?
There was a problem hiding this comment.
yes i forgot the incantation.
| @media screen and (max-width: 535px) { | ||
| .tooltip { | ||
| min-width: 45vw; | ||
| padding: 3vw; |
There was a problem hiding this comment.
feels a bit weird to scale the padding as well, looks pretty good though so 👍
| @@ -99,6 +99,39 @@ | |||
| </div> | |||
| </template> | |||
|
|
|||
|
|
|||
| <!-- Lighthouse score container --> | |||
| <template id="tmpl-lh-score-container"> | |||
There was a problem hiding this comment.
scores-container? score-container made me think it was for a single score :)
There was a problem hiding this comment.
re: devtools overriding. It definitely sucks having both overriding and inline conditionals for devtools to customize the report. My preference is overriding just cause that's how we designed, but would it be possible to bring in the overriding classes into the LH repo so the could all be here at once?
| @@ -61,9 +61,9 @@ class UsesHTTP2Audit extends Audit { | |||
| let displayValue = ''; | |||
| if (resources.length > 1) { | |||
| displayValue = | |||
| `${Util.formatNumber(resources.length)} requests were not handled over HTTP/2`; | |||
| `${Util.formatNumber(resources.length)} requests not served via HTTP/2`; | |||
There was a problem hiding this comment.
I deliberately omitted it for brevity. If it were a sentence i'd keep 'were' but it's just a lil string on the end of the line. wdyt
There was a problem hiding this comment.
I don't care much, but it is a little weird since the audit touched above keeps the verb :)
| @@ -61,18 +61,33 @@ class ReportRenderer { | |||
| */ | |||
| _renderReportHeader(report) { | |||
| const header = this._dom.cloneTemplate('#tmpl-lh-heading', this._templateContext); | |||
| const scoreContainer = this._dom.cloneTemplate('#tmpl-lh-score-container', this._templateContext); | |||
| const toolbarEl = this._dom.find('.lh-toolbar', header); | |||
| /** @type {HTMLDivElement} */ (toolbarEl.parentNode).insertBefore(scoreContainer, toolbarEl); | |||
There was a problem hiding this comment.
can the template just have a slot for the score container?
That's a good idea. I can look into that after this guy. |
SG |
| /** | ||
| * @return {boolean} | ||
| */ | ||
| isDevTools() { |
There was a problem hiding this comment.
add a TODO (maybe here?) for the future DT override cleanup?
No description provided.