Skip to content
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.

Sign up for GitHub

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

Jump to bottom

report: DRY up audit & opportunity rendering #5136

Merged
merged 9 commits into from
May 17, 2018
Merged

report: DRY up audit & opportunity rendering #5136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d538df8
resolve failing tests.
paulirish May 8, 2018
c1359fc
feedback
paulirish May 11, 2018
7e2e5aa
test failure.
paulirish May 12, 2018
bc2c815
feedback from brendan.
paulirish May 16, 2018
e1f5ba2
error audits don't have wastedMs
paulirish May 16, 2018
87b782c
safety first.
paulirish May 16, 2018
c4ec87e
Merge branch 'master' into oppoAsAnAudit
paulirish May 17, 2018
59b9e49
fix typecheck
paulirish May 17, 2018
cd48fd0
feedback.
paulirish May 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions lighthouse-core/report/html/renderer/category-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,17 @@ class CategoryRenderer {
*/
renderAudit(audit, index) {
const tmpl = this.dom.cloneTemplate('#tmpl-lh-audit', this.templateContext);
return this.populateAuditValues(audit, index, tmpl);
}

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still want a docstring here :P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* Populate an DOM tree with audit details. Used by renderAudit and renderOpportunity
* @param {AuditJSON} audit
* @param {number} index
* @param {DocumentFragment} tmpl
* @return {Element}
*/
populateAuditValues(audit, index, tmpl) {
const auditEl = this.dom.find('.lh-audit', tmpl);
auditEl.id = audit.result.id;
const scoreDisplayMode = audit.result.scoreDisplayMode;
Expand All @@ -49,19 +60,18 @@ class CategoryRenderer {

const titleEl = this.dom.find('.lh-audit__title', auditEl);
titleEl.appendChild(this.dom.convertMarkdownCodeSnippets(audit.result.title));
this.dom.find('.lh-audit__description', auditEl)
.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description));
if (audit.result.description) {
this.dom.find('.lh-audit__description', auditEl)
.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description));
}

// Append audit details to header section so the entire audit is within a <details>.
const header = /** @type {HTMLDetailsElement} */ (this.dom.find('details', auditEl));
if (audit.result.details && audit.result.details.type) {
const elem = this.detailsRenderer.render(audit.result.details);
elem.classList.add('lh-details');
header.appendChild(elem);
}

auditEl.classList.add(`lh-audit--${audit.result.scoreDisplayMode}`);

this.dom.find('.lh-audit__index', auditEl).textContent = `${index + 1}`;

this._setRatingClass(auditEl, audit.result.score, scoreDisplayMode);
Expand All @@ -71,7 +81,7 @@ class CategoryRenderer {
const textEl = this.dom.find('.lh-audit__display-text', auditEl);
textEl.textContent = 'Error!';
textEl.classList.add('tooltip-boundary');
const tooltip = this.dom.createChildOf(textEl, 'div', 'lh-error-tooltip-content tooltip');
const tooltip = this.dom.createChildOf(textEl, 'div', 'tooltip lh-debug');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make sense for the tooltip to share the lh-debug class?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

tooltip.textContent = audit.result.errorMessage || 'Report error: no audit information';
} else if (audit.result.explanation) {
const explanationEl = this.dom.createChildOf(titleEl, 'div', 'lh-debug');
Expand Down
43 changes: 14 additions & 29 deletions lighthouse-core/report/html/renderer/performance-category-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
if (audit.result.scoreDisplayMode === 'error') {
descriptionEl.textContent = '';
valueEl.textContent = 'Error!';
const tooltip = this.dom.createChildOf(descriptionEl, 'span', 'lh-error-tooltip-content');
const tooltip = this.dom.createChildOf(descriptionEl, 'span');
tooltip.textContent = audit.result.errorMessage || 'Report error: no metric information';
}

Expand All @@ -52,45 +52,30 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
* @return {Element}
*/
_renderOpportunity(audit, index, scale) {
const tmpl = this.dom.cloneTemplate('#tmpl-lh-opportunity', this.templateContext);
const element = this.dom.find('.lh-load-opportunity', tmpl);
element.classList.add(`lh-load-opportunity--${Util.calculateRating(audit.result.score)}`);
const oppTmpl = this.dom.cloneTemplate('#tmpl-lh-opportunity', this.templateContext);
const element = this.populateAuditValues(audit, index, oppTmpl);
element.id = audit.result.id;

const titleEl = this.dom.find('.lh-load-opportunity__title', tmpl);
titleEl.textContent = audit.result.title;
this.dom.find('.lh-audit__index', element).textContent = `${index + 1}`;

if (audit.result.errorMessage || audit.result.explanation) {
const debugStrEl = this.dom.createChildOf(titleEl, 'div', 'lh-debug');
debugStrEl.textContent = audit.result.errorMessage || audit.result.explanation || null;
}
if (audit.result.scoreDisplayMode === 'error') return element;

const details = audit.result.details;
if (!details) {
return element;
}
const summaryInfo = /** @type {OpportunitySummary} */ (details.summary);
if (!summaryInfo || !summaryInfo.wastedMs) {
if (!summaryInfo || !summaryInfo.wastedMs || audit.result.scoreDisplayMode === 'error') {
return element;
}

const displayValue = Util.formatDisplayValue(audit.result.displayValue);
// Overwrite the displayValue with opportunity's wastedMs
const displayEl = this.dom.find('.lh-audit__display-text', element);
const sparklineWidthPct = `${summaryInfo.wastedMs / scale * 100}%`;
const wastedMs = Util.formatSeconds(summaryInfo.wastedMs, 0.01);
const auditDescription = this.dom.convertMarkdownLinkSnippets(audit.result.description);
this.dom.find('.lh-load-opportunity__sparkline', tmpl).title = displayValue;
this.dom.find('.lh-load-opportunity__wasted-stat', tmpl).title = displayValue;
this.dom.find('.lh-sparkline__bar', tmpl).style.width = sparklineWidthPct;
this.dom.find('.lh-load-opportunity__wasted-stat', tmpl).textContent = wastedMs;
this.dom.find('.lh-load-opportunity__description', tmpl).appendChild(auditDescription);

// If there's no `type`, then we only used details for `summary`
if (details.type) {
const detailsElem = this.detailsRenderer.render(details);
detailsElem.classList.add('lh-details');
element.appendChild(detailsElem);
this.dom.find('.lh-sparkline__bar', element).style.width = sparklineWidthPct;
displayEl.textContent = Util.formatSeconds(summaryInfo.wastedMs, 0.01);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels weird to overwrite Error! with this in the error case. Will it always have a summaryInfo if there was a gatherer or audit error?


// Set [title] tooltips
if (audit.result.displayValue) {
const displayValue = Util.formatDisplayValue(audit.result.displayValue);
this.dom.find('.lh-load-opportunity__sparkline', element).title = displayValue;
displayEl.title = displayValue;
}

return element;
Expand Down
6 changes: 3 additions & 3 deletions lighthouse-core/report/html/renderer/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,19 @@ class Util {
* @return {string}
*/
static formatDisplayValue(displayValue) {
if (typeof displayValue === 'undefined') return '';
if (typeof displayValue === 'string') return displayValue;
if (!displayValue) return '';

const replacementRegex = /%([0-9]*(\.[0-9]+)?d|s)/;
const template = /** @type {string} */ (displayValue.shift());
const template = /** @type {string} */ (displayValue[0]);
if (typeof template !== 'string') {
// First value should always be the format string, but we don't want to fail to build
// a report, return a placeholder.
return 'UNKNOWN';
}

let output = template;
for (const replacement of displayValue) {
for (const replacement of displayValue.slice(1)) {
if (!replacementRegex.test(output)) {
// eslint-disable-next-line no-console
console.warn('Too many replacements given');
Expand Down
118 changes: 39 additions & 79 deletions lighthouse-core/report/html/report-styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,6 @@
color: #0c50c7;
}

.lh-root details summary {
cursor: pointer;
}

.lh-audit__description,
.lh-load-opportunity__description,
Expand Down Expand Up @@ -259,14 +256,19 @@
margin: calc(var(--default-padding) / 2) 0 var(--default-padding);
}

.lh-audit__header > div,
.lh-audit__header > span {

.lh-audit__index,
.lh-audit__title,
.lh-audit__display-text,
.lh-audit__score-icon,
.lh-load-opportunity__sparkline,
.lh-toggle-arrow {
margin: 0 var(--audit-item-gap);
}
.lh-audit__header > div:first-child, .lh-audit__header > span:first-child {
.lh-audit__index {
margin-left: 0;
}
.lh-audit__header > div:last-child, .lh-audit__header > span:last-child {
.lh-toggle-arrow {
margin-right: 0;
}

Expand Down Expand Up @@ -296,27 +298,25 @@
}

/* Expandable Details (Audit Groups, Audits) */

.lh-expandable-details {

}

.lh-expandable-details__summary {
cursor: pointer;
}

.lh-audit__header {
display: flex;
padding: var(--lh-audit-vpadding) var(--text-indent);
cursor: pointer;
}

.lh-audit--load-opportunity .lh-audit__header {
display: block;
}

.lh-audit__header:hover {
background-color: #F8F9FA;
}


.lh-audit-group[open] > .lh-audit-group__summary > .lh-toggle-arrow,
.lh-expandable-details[open] > .lh-expandable-details__summary > .lh-toggle-arrow,
.lh-expandable-details[open] > .lh-expandable-details__summary > div > .lh-toggle-arrow {
.lh-expandable-details[open] > .lh-expandable-details__summary > div > .lh-toggle-arrow,
.lh-expandable-details[open] > .lh-expandable-details__summary > div > div > .lh-toggle-arrow {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the nest grows deeper :( removed in chevron ✨ though?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yah in #5137 this gets cleaned up a lot.

transform: rotateZ(-90deg);
}

Expand Down Expand Up @@ -428,14 +428,6 @@

/* Perf load opportunity */

.lh-load-opportunity {
border-bottom: 1px solid var(--report-secondary-border-color);
}

.lh-load-opportunity:last-of-type {
border-bottom: none;
}

.lh-load-opportunity__cols {
display: flex;
align-items: flex-start;
Expand All @@ -449,20 +441,11 @@
line-height: calc(2.3 * var(--body-font-size));
}

.lh-load-opportunity__summary {
padding: var(--lh-audit-vpadding) var(--text-indent);
}
.lh-load-opportunity__summary:hover {
background-color: #F8F9FA;
}

.lh-load-opportunity__col {
display: flex;
justify-content: space-between;
}
.lh-load-opportunity__col > * {
margin: 0 5px;
}

.lh-load-opportunity__col--one {
flex: 5;
margin-right: 2px;
Expand All @@ -471,46 +454,9 @@
flex: 4;
}

.lh-load-opportunity__title {
font-size: var(--body-font-size);
flex: 10;
}


.lh-load-opportunity__wasted-stat {
.lh-audit--load-opportunity .lh-audit__display-text {
text-align: right;
flex: 0 0 calc(3 * var(--body-font-size));
font-size: var(--body-font-size);
line-height: var(--body-line-height);
}

.lh-load-opportunity__description {
color: var(--secondary-text-color);
margin-top: calc(var(--default-padding) / 2);
}

.lh-load-opportunity--pass .lh-load-opportunity__wasted-stat {
color: var(--pass-color);
}

.lh-load-opportunity--pass .lh-sparkline__bar {
background: var(--pass-color);
}

.lh-load-opportunity--average .lh-sparkline__bar {
background: var(--average-color);
}

.lh-load-opportunity--average .lh-load-opportunity__wasted-stat {
color: var(--average-color);
}

.lh-load-opportunity--fail .lh-sparkline__bar {
background: var(--fail-color);
}

.lh-load-opportunity--fail .lh-load-opportunity__wasted-stat {
color: var(--fail-color);
}


Expand All @@ -531,6 +477,19 @@
float: right;
}

.lh-audit--pass .lh-sparkline__bar {
background: var(--pass-color);
}

.lh-audit--average .lh-sparkline__bar {
background: var(--average-color);
}

.lh-audit--fail .lh-sparkline__bar {
background: var(--fail-color);
}



/* Filmstrip */

Expand Down Expand Up @@ -562,7 +521,6 @@

.lh-audit {
border-bottom: 1px solid var(--report-secondary-border-color);
font-size: var(--body-font-size);
}

.lh-audit:last-child {
Expand Down Expand Up @@ -664,7 +622,7 @@
font-size: var(--caption-font-size);
line-height: var(--caption-line-height);
color: var(--fail-color);
margin-top: 3px;
margin-bottom: 3px;
}


Expand Down Expand Up @@ -973,6 +931,12 @@ summary.lh-passed-audits-summary {
position: absolute;
display: none; /* Don't retain these layers when not needed */
opacity: 0;

background: #ffffff;
min-width: 23em;
padding: 15px;
border-radius: 5px;
text-align: initial;
}

.tooltip-boundary:hover {
Expand All @@ -984,10 +948,6 @@ summary.lh-passed-audits-summary {
animation: fadeInTooltip 250ms;
animation-fill-mode: forwards;
animation-delay: 850ms;
min-width: 23em;
background: #ffffff;
padding: 15px;
border-radius: 5px;
bottom: 100%;
z-index: 1;
will-change: opacity;
Expand Down
Loading