-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
report: final metrics display, icons, whitespace polish #5130
Changes from all commits
b01434d
094ac84
243177b
2036ce7
a4c6b44
8201509
2cfba17
f34698e
7bb3b54
86e5271
91d1f63
36ab00a
ec5ed49
0a579a1
6eda99d
34d0e24
ef57acd
f668318
f1701e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,8 +39,8 @@ class CategoryRenderer { | |
this.dom.find('.lh-audit__display-text', auditEl).textContent = displayValue; | ||
} | ||
|
||
this.dom.find('.lh-audit__title', auditEl).appendChild( | ||
this.dom.convertMarkdownCodeSnippets(audit.result.description)); | ||
const titleEl = this.dom.find('.lh-audit__title', auditEl); | ||
titleEl.appendChild(this.dom.convertMarkdownCodeSnippets(audit.result.description)); | ||
this.dom.find('.lh-audit__description', auditEl) | ||
.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.helpText)); | ||
|
||
|
@@ -64,7 +64,7 @@ class CategoryRenderer { | |
const tooltip = this.dom.createChildOf(textEl, 'div', 'lh-error-tooltip-content tooltip'); | ||
tooltip.textContent = audit.result.debugString || 'Report error: no audit information'; | ||
} else if (audit.result.debugString) { | ||
const debugStrEl = auditEl.appendChild(this.dom.createElement('div', 'lh-debug')); | ||
const debugStrEl = this.dom.createChildOf(titleEl, 'div', 'lh-debug'); | ||
debugStrEl.textContent = audit.result.debugString; | ||
} | ||
return auditEl; | ||
|
@@ -86,7 +86,7 @@ class CategoryRenderer { | |
* @param {!ReportRenderer.CategoryJSON} category | ||
* @return {!Element} | ||
*/ | ||
renderCategoryScore(category) { | ||
renderCategoryHeader(category) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 👍 |
||
const tmpl = this.dom.cloneTemplate('#tmpl-lh-category-header', this.templateContext); | ||
|
||
const gaugeContainerEl = this.dom.find('.lh-score__gauge', tmpl); | ||
|
@@ -95,8 +95,11 @@ class CategoryRenderer { | |
|
||
this.dom.find('.lh-category-header__title', tmpl).appendChild( | ||
this.dom.convertMarkdownCodeSnippets(category.name)); | ||
this.dom.find('.lh-category-header__description', tmpl) | ||
.appendChild(this.dom.convertMarkdownLinkSnippets(category.description)); | ||
if (category.description) { | ||
const descEl = this.dom.convertMarkdownLinkSnippets(category.description); | ||
this.dom.find('.lh-category-header__description', tmpl).appendChild(descEl); | ||
} | ||
|
||
|
||
return /** @type {!Element} */ (tmpl.firstElementChild); | ||
} | ||
|
@@ -157,9 +160,7 @@ class CategoryRenderer { | |
* @return {!Element} | ||
*/ | ||
_renderFailedAuditsSection(elements) { | ||
const failedElem = this.renderAuditGroup({ | ||
title: `Failed audits`, | ||
}, {expandable: false, itemCount: this._getTotalAuditsLength(elements)}); | ||
const failedElem = this.dom.createElement('div'); | ||
failedElem.classList.add('lh-failed-audits'); | ||
elements.forEach(elem => failedElem.appendChild(elem)); | ||
return failedElem; | ||
|
@@ -253,7 +254,7 @@ class CategoryRenderer { | |
render(category, groupDefinitions) { | ||
const element = this.dom.createElement('div', 'lh-category'); | ||
this.createPermalinkSpan(element, category.id); | ||
element.appendChild(this.renderCategoryScore(category)); | ||
element.appendChild(this.renderCategoryHeader(category)); | ||
|
||
const manualAudits = category.audits.filter(item => item.result.scoreDisplayMode === 'manual'); | ||
const nonManualAudits = category.audits.filter(audit => !manualAudits.includes(audit)); | ||
|
@@ -300,43 +301,37 @@ class CategoryRenderer { | |
auditsUngrouped.notApplicable.forEach((/** @type {!ReportRenderer.AuditJSON} */ audit, i) => | ||
notApplicableElements.push(this.renderAudit(audit, i))); | ||
|
||
let hasFailedGroups = false; | ||
|
||
Object.keys(auditsGroupedByGroup).forEach(groupId => { | ||
const group = groupDefinitions[groupId]; | ||
const groups = auditsGroupedByGroup[groupId]; | ||
|
||
if (groups.failed.length) { | ||
const auditGroupElem = this.renderAuditGroup(group, {expandable: false}); | ||
groups.failed.forEach((item, i) => auditGroupElem.appendChild(this.renderAudit(item, i))); | ||
auditGroupElem.classList.add('lh-audit-group--unadorned'); | ||
auditGroupElem.open = true; | ||
failedElements.push(auditGroupElem); | ||
|
||
hasFailedGroups = true; | ||
} | ||
|
||
if (groups.passed.length) { | ||
const auditGroupElem = this.renderAuditGroup(group, {expandable: true}); | ||
groups.passed.forEach((item, i) => auditGroupElem.appendChild(this.renderAudit(item, i))); | ||
auditGroupElem.classList.add('lh-audit-group--unadorned'); | ||
passedElements.push(auditGroupElem); | ||
} | ||
|
||
if (groups.notApplicable.length) { | ||
const auditGroupElem = this.renderAuditGroup(group, {expandable: true}); | ||
groups.notApplicable.forEach((item, i) => | ||
auditGroupElem.appendChild(this.renderAudit(item, i))); | ||
auditGroupElem.classList.add('lh-audit-group--unadorned'); | ||
notApplicableElements.push(auditGroupElem); | ||
} | ||
}); | ||
|
||
if (failedElements.length) { | ||
// if failed audits are grouped skip the 'X Failed Audits' header | ||
if (hasFailedGroups) { | ||
failedElements.forEach(elem => element.appendChild(elem)); | ||
} else { | ||
const failedElem = this._renderFailedAuditsSection(failedElements); | ||
element.appendChild(failedElem); | ||
} | ||
const failedElem = this._renderFailedAuditsSection(failedElements); | ||
element.appendChild(failedElem); | ||
} | ||
|
||
if (manualAudits.length) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,13 +50,12 @@ class PerformanceCategoryRenderer extends CategoryRenderer { | |
element.classList.add(`lh-load-opportunity--${Util.calculateRating(audit.result.score)}`); | ||
element.id = audit.result.name; | ||
|
||
const summary = this.dom.find('.lh-load-opportunity__summary', tmpl); | ||
const titleEl = this.dom.find('.lh-load-opportunity__title', tmpl); | ||
titleEl.textContent = audit.result.description; | ||
this.dom.find('.lh-audit__index', element).textContent = `${index + 1}`; | ||
|
||
if (audit.result.debugString || audit.result.error) { | ||
const debugStrEl = this.dom.createChildOf(summary, 'div', 'lh-debug'); | ||
const debugStrEl = this.dom.createChildOf(titleEl, 'div', 'lh-debug'); | ||
debugStrEl.textContent = audit.result.debugString || 'Audit error'; | ||
} | ||
if (audit.result.error) return element; | ||
|
@@ -86,18 +85,37 @@ class PerformanceCategoryRenderer extends CategoryRenderer { | |
return element; | ||
} | ||
|
||
/** | ||
* Get an audit's wastedMs to sort the opportunity by, and scale the sparkline width | ||
* Opportunties with an error won't have a summary object, so MIN_VALUE is returned to keep any | ||
* erroring opportunities last in sort order. | ||
* @param {!ReportRenderer.AuditJSON} audit | ||
* @return {number} | ||
*/ | ||
_getWastedMs(audit) { | ||
if ( | ||
audit.result.details && | ||
audit.result.details.summary && | ||
typeof audit.result.details.summary.wastedMs === 'number' | ||
) { | ||
return audit.result.details.summary.wastedMs; | ||
} else { | ||
return Number.MIN_VALUE; | ||
} | ||
} | ||
|
||
/** | ||
* @override | ||
*/ | ||
render(category, groups) { | ||
const element = this.dom.createElement('div', 'lh-category'); | ||
this.createPermalinkSpan(element, category.id); | ||
element.appendChild(this.renderCategoryScore(category)); | ||
element.appendChild(this.renderCategoryHeader(category)); | ||
|
||
// Metrics | ||
const metricAudits = category.audits.filter(audit => audit.group === 'metrics'); | ||
const metricAuditsEl = this.renderAuditGroup(groups['metrics'], {expandable: false}); | ||
|
||
// Metrics | ||
const keyMetrics = metricAudits.filter(a => a.weight >= 3); | ||
const otherMetrics = metricAudits.filter(a => a.weight < 3); | ||
|
||
|
@@ -111,9 +129,16 @@ class PerformanceCategoryRenderer extends CategoryRenderer { | |
otherMetrics.forEach(item => { | ||
metricsColumn2El.appendChild(this._renderMetric(item)); | ||
}); | ||
const estValuesEl = this.dom.createChildOf(metricsColumn2El, 'div', | ||
'lh-metrics__disclaimer lh-metrics__disclaimer'); | ||
estValuesEl.textContent = 'Values are estimated and may vary.'; | ||
|
||
metricAuditsEl.open = true; | ||
metricAuditsEl.classList.add('lh-audit-group--metrics'); | ||
element.appendChild(metricAuditsEl); | ||
|
||
// Filmstrip | ||
const timelineEl = this.dom.createChildOf(metricAuditsEl, 'div', 'lh-timeline'); | ||
const timelineEl = this.dom.createChildOf(element, 'div', 'lh-filmstrip-container'); | ||
const thumbnailAudit = category.audits.find(audit => audit.id === 'screenshot-thumbnails'); | ||
const thumbnailResult = thumbnailAudit && thumbnailAudit.result; | ||
if (thumbnailResult && thumbnailResult.details) { | ||
|
@@ -124,15 +149,14 @@ class PerformanceCategoryRenderer extends CategoryRenderer { | |
timelineEl.appendChild(filmstripEl); | ||
} | ||
|
||
metricAuditsEl.open = true; | ||
element.appendChild(metricAuditsEl); | ||
|
||
// Opportunities | ||
const opportunityAudits = category.audits | ||
.filter(audit => audit.group === 'load-opportunities' && !Util.showAsPassed(audit.result)) | ||
.sort((auditA, auditB) => auditB.result.rawValue - auditA.result.rawValue); | ||
.sort((auditA, auditB) => this._getWastedMs(auditB) - this._getWastedMs(auditA)); | ||
|
||
if (opportunityAudits.length) { | ||
const maxWaste = Math.max(...opportunityAudits.map(audit => audit.result.rawValue)); | ||
const wastedMsValues = opportunityAudits.map(audit => this._getWastedMs(audit)); | ||
const maxWaste = Math.max(...wastedMsValues); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it possible for this to be all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my preference for "real fix" would be to explicitly handle error cases separately so we can assume they're all regular sane numbers but for now, I have no strong preference other than clearly expressing a nonsense number in the error case, MIN_VALUE seemed more obviously nonsense then -1, if it creates too much other noise compared to -1 though, I'm fine with it |
||
const scale = Math.ceil(maxWaste / 1000) * 1000; | ||
const groupEl = this.renderAuditGroup(groups['load-opportunities'], {expandable: false}); | ||
const tmpl = this.dom.cloneTemplate('#tmpl-lh-opportunity-header', this.templateContext); | ||
|
@@ -141,6 +165,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer { | |
opportunityAudits.forEach((item, i) => | ||
groupEl.appendChild(this._renderOpportunity(item, i, scale))); | ||
groupEl.open = true; | ||
groupEl.classList.add('lh-audit-group--opportunities'); | ||
element.appendChild(groupEl); | ||
} | ||
|
||
|
@@ -157,9 +182,11 @@ class PerformanceCategoryRenderer extends CategoryRenderer { | |
const groupEl = this.renderAuditGroup(groups['diagnostics'], {expandable: false}); | ||
diagnosticAudits.forEach((item, i) => groupEl.appendChild(this.renderAudit(item, i))); | ||
groupEl.open = true; | ||
groupEl.classList.add('lh-audit-group--diagnostics'); | ||
element.appendChild(groupEl); | ||
} | ||
|
||
// Passed audits | ||
const passedElements = category.audits | ||
.filter(audit => (audit.group === 'load-opportunities' || audit.group === 'diagnostics') && | ||
Util.showAsPassed(audit.result)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive by restructuring? seems unrelated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Kinda separate but hey. I mentioned it on chat yesterday
before:
after: