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.

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

Adjust report-renderer init for devtools #2002

Merged
merged 8 commits into from
Apr 14, 2017
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
40 changes: 29 additions & 11 deletions lighthouse-core/report/v2/renderer/details-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/
'use strict';

/* globals self */

class DetailsRenderer {
/**
* @param {!DOM} dom
Expand Down Expand Up @@ -43,7 +45,7 @@ class DetailsRenderer {
}

/**
* @param {!DetailsJSON} text
* @param {!DetailsRenderer.DetailsJSON} text
* @return {!Element}
*/
_renderText(text) {
Expand All @@ -53,19 +55,20 @@ class DetailsRenderer {
}

/**
* @param {!DetailsJSON} block
* @param {!DetailsRenderer.DetailsJSON} block
* @return {!Element}
*/
_renderBlock(block) {
const element = this._dom.createElement('div', 'lh-block');
for (const item of block.items) {
const items = block.items || [];
for (const item of items) {
element.appendChild(this.render(item));
}
return element;
}

/**
* @param {!DetailsJSON} list
* @param {!DetailsRenderer.DetailsJSON} list
* @return {!Element}
*/
_renderList(list) {
Expand All @@ -76,11 +79,12 @@ class DetailsRenderer {
element.appendChild(summary);
}

const items = this._dom.createElement('div', 'lh-list__items');
for (const item of list.items) {
items.appendChild(this.render(item));
const itemsElem = this._dom.createElement('div', 'lh-list__items');
const items = list.items || [];
for (const item of items) {
itemsElem.appendChild(this.render(item));
}
element.appendChild(items);
element.appendChild(itemsElem);
return element;
}

Expand Down Expand Up @@ -117,11 +121,25 @@ class DetailsRenderer {

if (typeof module !== 'undefined' && module.exports) {
module.exports = DetailsRenderer;
} else {
self.DetailsRenderer = DetailsRenderer;
}

/** @typedef {{type: string, text: string|undefined, header: DetailsJSON|undefined, items: Array<DetailsJSON>|undefined}} */
/**
* @typedef {{
* type: string,
* text: (string|undefined),
* header: (DetailsRenderer.DetailsJSON|undefined),
Copy link
Member

Choose a reason for hiding this comment

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

(!DetailsRenderer.DetailsJSON|undefined)

* items: (!Array<!DetailsRenderer.DetailsJSON>|undefined)
* }}
*/
DetailsRenderer.DetailsJSON; // eslint-disable-line no-unused-expressions


/** @typedef {{type: string, text: string, header: DetailsJSON, items: Array<{title: string, value: string, snippet: string|undefined, target: string}>}} */
/** @typedef {{
* type: string,
* text: string,
* header: DetailsJSON,
Copy link
Member

Choose a reason for hiding this comment

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

!DetailsJSON

* items: Array<{title: string, value: string, snippet: string|undefined, target: string}>
Copy link
Member

Choose a reason for hiding this comment

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

!Array. did it need parens around snippet's type?

* }}
*/
DetailsRenderer.CardsDetailsJSON; // eslint-disable-line no-unused-expressions
23 changes: 17 additions & 6 deletions lighthouse-core/report/v2/renderer/dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/
'use strict';

/* globals URL */
/* globals URL self */

class DOM {
/**
Expand All @@ -33,7 +33,8 @@ class DOM {
* set the attribute on the node.
* @return {!Element}
*/
createElement(name, className, attrs = {}) {
createElement(name, className, attrs) {
attrs = attrs || {};
Copy link
Member

Choose a reason for hiding this comment

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

TODO switch this back when https://codereview.chromium.org/2821773002/ lands?

const element = this._document.createElement(name);
if (className) {
element.className = className;
Expand All @@ -48,20 +49,21 @@ class DOM {

/**
* @param {string} selector
* @param {!Document|!Element} context
* @return {!DocumentFragment} A clone of the template content.
* @throws {Error}
*/
cloneTemplate(selector) {
const template = this._document.querySelector(selector);
cloneTemplate(selector, context) {
const template = context.querySelector(selector);
if (!template) {
throw new Error(`Template not found: template${selector}`);
}
return this._document.importNode(template.content, true);
return /** @type {!DocumentFragment} */ (this._document.importNode(template.content, true));
}

/**
* @param {string} text
* @return {!HTMLSpanElement}
* @return {!Element}
*/
createSpanFromMarkdown(text) {
const element = this.createElement('span');
Expand All @@ -87,8 +89,17 @@ class DOM {

return element;
}

/**
* @return {!Document}
*/
document() {
return this._document;
}
}

if (typeof module !== 'undefined' && module.exports) {
module.exports = DOM;
} else {
self.DOM = DOM;
}
82 changes: 62 additions & 20 deletions lighthouse-core/report/v2/renderer/report-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
* Dummy text for ensuring report robustness: </script> pre$`post %%LIGHTHOUSE_JSON%%
*/

/* globals DOM, DetailsRenderer */
/* globals self */

const RATINGS = {
PASS: {label: 'pass', minScore: 75},
Expand Down Expand Up @@ -56,15 +56,18 @@ function formatNumber(number) {

class ReportRenderer {
/**
* @param {!Document} document
* @param {!DOM} dom
* @param {!DetailsRenderer} detailsRenderer
*/
constructor(document) {
this._dom = new DOM(document);
this._detailsRenderer = new DetailsRenderer(this._dom);
constructor(dom, detailsRenderer) {
this._dom = dom;
this._detailsRenderer = detailsRenderer;

this._templateContext = this._dom.document();
}

/**
* @param {!ReportJSON} report
* @param {!ReportRenderer.ReportJSON} report
* @return {!Element}
*/
renderReport(report) {
Expand Down Expand Up @@ -94,15 +97,22 @@ class ReportRenderer {
element.querySelector('.lh-score__description')
.appendChild(this._dom.createSpanFromMarkdown(description));

return element;
return /** @type {!Element} **/ (element);
}

/**
* @param {!Document|!Element} context
Copy link
Member

Choose a reason for hiding this comment

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

maybe a function description for why you'd want to inject this? there won't be any hints on the LH side

*/
setTemplateContext(context) {
this._templateContext = context;
}

/**
* @param {!AuditJSON} audit
* @param {!ReportRenderer.AuditJSON} audit
* @return {!Element}
*/
_renderAuditScore(audit) {
const tmpl = this._dom.cloneTemplate('#tmpl-lh-audit-score');
const tmpl = this._dom.cloneTemplate('#tmpl-lh-audit-score', this._templateContext);

const scoringMode = audit.result.scoringMode;
const description = audit.result.helpText;
Expand All @@ -126,11 +136,11 @@ class ReportRenderer {
}

/**
* @param {!CategoryJSON} category
* @param {!ReportRenderer.CategoryJSON} category
* @return {!Element}
*/
_renderCategoryScore(category) {
const tmpl = this._dom.cloneTemplate('#tmpl-lh-category-score');
const tmpl = this._dom.cloneTemplate('#tmpl-lh-category-score', this._templateContext);
const score = Math.round(category.score);
return this._populateScore(tmpl, score, 'numeric', category.name, category.description);
}
Expand All @@ -146,7 +156,7 @@ class ReportRenderer {
}

/**
* @param {!ReportJSON} report
* @param {!ReportRenderer.ReportJSON} report
* @return {!Element}
*/
_renderReport(report) {
Expand All @@ -158,7 +168,7 @@ class ReportRenderer {
}

/**
* @param {!CategoryJSON} category
* @param {!ReportRenderer.CategoryJSON} category
* @return {!Element}
*/
_renderCategory(category) {
Expand All @@ -185,7 +195,7 @@ class ReportRenderer {
}

/**
* @param {!AuditJSON} audit
* @param {!ReportRenderer.AuditJSON} audit
* @return {!Element}
*/
_renderAudit(audit) {
Expand All @@ -197,13 +207,45 @@ class ReportRenderer {

if (typeof module !== 'undefined' && module.exports) {
module.exports = ReportRenderer;
} else {
self.ReportRenderer = ReportRenderer;
}

/** @typedef {{id: string, weight: number, score: number, result: {description: string, displayValue: string, helpText: string, score: number|boolean, details: DetailsRenderer.DetailsJSON|DetailsRenderer.CardsDetailsJSON|undefined}}} */
let AuditJSON; // eslint-disable-line no-unused-vars
/**
* @typedef {{
* id: string, weight:
* number, score: number,
* result: {
* description: string,
* displayValue: string,
* helpText: string,
* score: (number|boolean),
* scoringMode: string,
* details: (DetailsRenderer.DetailsJSON|DetailsRenderer.CardsDetailsJSON|undefined)
Copy link
Member

Choose a reason for hiding this comment

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

(!DetailsRenderer.DetailsJSON|!DetailsRenderer.CardsDetailsJSON|undefined) I think

* }
* }}
*/
ReportRenderer.AuditJSON; // eslint-disable-line no-unused-expressions

/** @typedef {{name: string, weight: number, score: number, description: string, audits: Array<AuditJSON>}} */
let CategoryJSON; // eslint-disable-line no-unused-vars
/**
* @typedef {{
* name: string,
* weight: number,
* score: number,
* description: string,
* audits: Array<ReportRenderer.AuditJSON>
Copy link
Member

Choose a reason for hiding this comment

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

!Array<!ReportRenderer.AuditJSON>

* }}
*/
ReportRenderer.CategoryJSON; // eslint-disable-line no-unused-expressions

/** @typedef {{reportCategories: Array<CategoryJSON>}} */
let ReportJSON; // eslint-disable-line no-unused-vars
/**
* @typedef {{
* lighthouseVersion: !string,
* generatedTime: !string,
* initialUrl: !string,
* url: !string,
* audits: ?Object,
* reportCategories: !Array<!ReportRenderer.CategoryJSON>
* }}
*/
ReportRenderer.ReportJSON; // eslint-disable-line no-unused-expressions
7 changes: 5 additions & 2 deletions lighthouse-core/report/v2/report-template.html
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,11 @@
<script>%%LIGHTHOUSE_JAVASCRIPT%%</script>
<script>window.__LIGHTHOUSE_JSON__ = %%LIGHTHOUSE_JSON%%;</script>
<script>
const renderer = new ReportRenderer(document);
document.body.appendChild(renderer.renderReport(window.__LIGHTHOUSE_JSON__));
const dom = new DOM(document);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, DI! I feel like we'll have to move more implementation out as time goes on. I guess it's fine for now.

const detailsRenderer = new DetailsRenderer(dom);
const renderer = new ReportRenderer(dom, detailsRenderer);
const reportElem = renderer.renderReport(window.__LIGHTHOUSE_JSON__);
document.body.appendChild(reportElem);
</script>
</body>
</html>
8 changes: 6 additions & 2 deletions lighthouse-core/test/report/v2/renderer/dom-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,16 @@ describe('DOM', () => {

describe('cloneTemplate', () => {
it('should clone a template', () => {
const clone = dom.cloneTemplate('#tmpl-lh-audit-score');
const clone = dom.cloneTemplate('#tmpl-lh-audit-score', dom.document());
assert.ok(clone.querySelector('.lh-score'));
});

it('fails when template cannot be found', () => {
assert.throws(() => dom.cloneTemplate('#unknown-selector'));
assert.throws(() => dom.cloneTemplate('#unknown-selector', dom.document()));
});

it('fails when a template context isn\'t provided', () => {
assert.throws(() => dom.cloneTemplate('#tmpl-lh-audit-score'));
});
});

Expand Down
16 changes: 11 additions & 5 deletions lighthouse-core/test/report/v2/renderer/report-renderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,14 @@ describe('ReportRenderer V2', () => {

before(() => {
global.URL = URL;
global.DOM = DOM;
global.DetailsRenderer = DetailsRenderer;
const document = jsdom.jsdom(TEMPLATE_FILE);
renderer = new ReportRenderer(document);
const dom = new DOM(document);
const detailsRenderer = new DetailsRenderer(dom);
renderer = new ReportRenderer(dom, detailsRenderer);
});

after(() => {
global.URL = undefined;
global.DOM = undefined;
global.DetailsRenderer = undefined;
});

describe('renderReport', () => {
Expand Down Expand Up @@ -96,4 +94,12 @@ describe('ReportRenderer V2', () => {
assert.equal(audits.length, category.audits.length, 'renders correct number of audits');
});
});

it('can set a custom templateContext', () => {
assert.equal(renderer._templateContext, renderer._dom.document());

const otherDocument = jsdom.jsdom(TEMPLATE_FILE);
renderer.setTemplateContext(otherDocument);
assert.equal(renderer._templateContext, otherDocument);
});
});