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
+126 −144
Merged

report: DRY up audit & opportunity rendering #5136

Show file tree
Hide file tree
Changes from 3 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: 17 additions & 5 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 @@ -30,21 +30,33 @@ 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

* @param {!ReportRenderer.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 displayTextEl = this.dom.find('.lh-audit__display-text', auditEl);
Copy link
Member

Choose a reason for hiding this comment

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

nit: move this into the conditional where it's used?

const scoreDisplayMode = audit.result.scoreDisplayMode;

if (audit.result.displayValue) {
const displayValue = Util.formatDisplayValue(audit.result.displayValue);
this.dom.find('.lh-audit__display-text', auditEl).textContent = displayValue;
displayTextEl.textContent = displayValue;
}

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);
Expand All @@ -63,7 +75,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
44 changes: 15 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 @@ -31,7 +31,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 @@ -45,43 +45,29 @@ 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.classList.add(`lh-audit--${Util.calculateRating(audit.result.score)}`);
Copy link
Member

Choose a reason for hiding this comment

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

looks like this is now already set in populateAuditValues by _setRatingClass()

Copy link
Member

Choose a reason for hiding this comment

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

lol populateAuditValues also has a stray auditEl.classList.add(lh-audit--${audit.result.scoreDisplayMode}); line right before calling _setRatingClass

Copy link
Member Author

Choose a reason for hiding this comment

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

hah. thanks for catching both of these

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;
}
if (audit.result.scoreDisplayMode === 'error') return element;

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

const displayValue = Util.formatDisplayValue(audit.result.displayValue);
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);
const wastedEl = this.dom.find('.lh-audit__display-text', element);
Copy link
Member

Choose a reason for hiding this comment

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

maybe comment that everything below here is overwriting the default displayValue display provided by populateAuditValues with opportunity-specific stuff?

if (audit.result.scoreDisplayMode !== 'error') {
const sparklineWidthPct = `${summaryInfo.wastedMs / scale * 100}%`;
this.dom.find('.lh-sparkline__bar', element).style.width = sparklineWidthPct;
wastedEl.textContent = Util.formatSeconds(summaryInfo.wastedMs, 0.01);
}

if (audit.result.displayValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

so if we're overriding displayValue anyhow and there's no helpText, we're using renderAudit for

  • populating index
  • setting rating class
  • sticking it inside of details
  • filling error/debugString
  • something else I'm missing?

WDYT about extracting those 4 to a method they both share?

Copy link
Member Author

Choose a reason for hiding this comment

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

i like the sound of that. will do.

Copy link
Member

Choose a reason for hiding this comment

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

this seems weird to do if it was an error scoreDisplayMode. Should be nested inside above error conditional?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks. didnt need the error conditional after all.

const displayValue = Util.formatDisplayValue(audit.result.displayValue);
this.dom.find('.lh-load-opportunity__sparkline', element).title = displayValue;
wastedEl.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 @@ -36,19 +36,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
34 changes: 18 additions & 16 deletions lighthouse-core/report/html/templates.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,24 +54,26 @@

<!-- Lighthouse perf opportunity -->
<template id="tmpl-lh-opportunity">
<details class="lh-load-opportunity lh-expandable-details">
<summary class="lh-load-opportunity__summary lh-expandable-details__summary">
<div class="lh-load-opportunity__cols">
<div class="lh-load-opportunity__col lh-load-opportunity__col--one">
<span class="lh-audit__index"></span>
<div class="lh-load-opportunity__title"></div>
</div>
<div class="lh-load-opportunity__col lh-load-opportunity__col--two">
<div class="lh-load-opportunity__sparkline">
<div class="lh-sparkline"><div class="lh-sparkline__bar"></div></div>
<div class="lh-audit lh-audit--load-opportunity">
<details class="lh-expandable-details">
<summary class="lh-audit__header lh-expandable-details__summary">
<div class="lh-load-opportunity__cols">
<div class="lh-load-opportunity__col lh-load-opportunity__col--one">
<span class="lh-audit__index"></span>
<div class="lh-audit__title"></div>
</div>
<div class="lh-load-opportunity__col lh-load-opportunity__col--two">
<div class="lh-load-opportunity__sparkline">
<div class="lh-sparkline"><div class="lh-sparkline__bar"></div></div>
</div>
<div class="lh-audit__display-text"></div>
<div class="lh-toggle-arrow" title="See resources"></div>
</div>
<div class="lh-load-opportunity__wasted-stat"></div>
<div class="lh-toggle-arrow" title="See resources"></div>
</div>
</div>
</summary>
<div class="lh-load-opportunity__description"></div>
</details>
</summary>
<div class="lh-audit__description"></div>
</details>
</div>
</template>


Expand Down
Loading