Add basic rendering to report generator v2 by dgozman · Pull Request #1933 · GoogleChrome/lighthouse

dgozman · 2017-03-29T19:15:40Z

Supports categories, audits, scores, lists, blocks and text.
Changed is-on-https audit to show list of urls as an example.
Clearly needs some design love.

Ref #1887

Supports categories, audits, scores, lists, blocks and text. Changed is-on-https audit to show list of urls as an example. Clearly needs some design love.

paulirish · 2017-03-29T19:20:42Z

FYI y'all: it's very basic.

Screenshot:

dgozman · 2017-03-29T20:21:57Z

lighthouse-core/report/v2/report-renderer.js

 };

+// /** @typedef {{type: string, text: string|undefined, header: DetailsJSON|undefined, items: Array<DetailsJSON>|undefined}} */
+// var DetailsJSON;


Linter doesn't like var here, but using let will pollute the scope. Is there a known way for typedefs?

Linter doesn't like var here, but using let will pollute the scope. Is there a known way for typedefs?

yeah, I was thinking about this when looking at enabling closure compilation of the project again. If we do more typedefs in files just meant for node/browserification, the nice thing is that those will be limited in scope to the file. For stuff in the browser it's harder. A few ideas:

we could add an .eslintrc to this directory to modify the rules, since in theory this js file will only ever run in a browser context and not in node (never say never, though :)

NTI doesn't support typedefs as namespaces, but would doing ReportRenderer.DetailsJSON work here since the namespace would be a real object?

we live with the pollution and in the future add something to report-generator.js that strips it out

We live with the pollution and pick names that will (probably) only ever be used for typedefs

My vote is for 4. The pollution seems very manageable, for the limited number of typedefs we have.

My vote is for 4

SGTM2.

We were also just talking and realized it doesn't matter if this is var or let here anyways, since even if we use var here if somewhere else a global with the same name is declared with let or const it will throw an error. So I'm good with let and global pollution.

patrickhulce · 2017-03-29T20:14:08Z

lighthouse-core/report/v2/report-styles.css

+
+/* Text */
+
+.lighthouse-text {


We tried to follow BEM in the previous report, do we have any opinions about it in the new world?

I'm personally not a huge fan, but I like the idea of having some convention to standardize around.

shadow dom is the solution :)

Agree with having a convention. That said, I don't love any of the options. But I think we need to pick one regardless

Honestly BEM probably has the most tooling support of any CSS convention, so I favor that.

Should we migrate to it in this PR? I could go either way.

Adopted BEM.

patrickhulce · 2017-03-29T20:15:36Z

lighthouse-core/report/v2/report-renderer.js

-    element.appendChild(pre);
+  renderReport(report) {
+    try {
+      var element = this._renderReport(report);


might have to make a decision if we follow devtools style and conventions or lighthouse (or drift the rest of lighthouse to devtools?). use of const, let vs. var, object shorthand, etc

patrickhulce · 2017-03-29T20:21:38Z

lighthouse-core/report/v2/report-renderer.js

+   */
+  _renderException(e) {
+    var element = this._createElement('div', 'lighthouse-exception');
+    element.textContent = '' + e;


thoughts on e.stack instead for a little more info?

paulirish · 2017-03-29T22:08:38Z

lighthouse-core/report/v2/report-renderer.js

+   */
+  _renderException(e) {
+    const element = this._createElement('div', 'lighthouse-exception');
+    element.textContent = String(e);


String(e.stack)

so we can get the message and callstack.

brendankenny · 2017-03-29T23:17:23Z

lighthouse-core/report/v2/report-renderer.js

 };

+/** @typedef {{type: string, text: string|undefined, header: DetailsJSON|undefined, items: Array<DetailsJSON>|undefined}} */
+let DetailsJSON;


can do let DetailsJSON; // eslint-disable-line no-unused-vars for each of these to disable the lint failure on just these lines

ebidel · 2017-03-29T20:55:40Z

lighthouse-core/report/v2/report-renderer.js

+   * @param {string} text
+   * @return {Node}
+   */
+  _createTextChild(parent, text) {


is this used anywhere?

Nope. I'll remove it. Thanks!

ebidel · 2017-03-29T21:05:46Z

lighthouse-core/report/v2/report-generator.js

  generateReportHtml(reportAsJson) {
    const sanitizedJson = JSON.stringify(reportAsJson).replace(/</g, '\\u003c');
    const sanitizedJavascript = REPORT_JAVASCRIPT.replace(/<\//g, '\\u003c/');
+    const sanitizedCss = REPORT_CSS.replace(/<\//g, '\\u003c/');


I think @brendankenny mentioned this in another PR, but sanitizedJavascript and sanitizedCss aren't necessary b/c we control both files. Might be a good time to just remove.

@ebidel I responded to that. This wasn't for XSS, this was for future-proof against us somewhere using </script> in a string and scratching heads for why the whole report is broken. We do it currently too.

Can rename to escaped or something if we want to make that clearer

We'd know right away if the whole report was broken :) The current _escapeScriptTags does the same as your sanitizedJson, which sanitizes user input in the json results. But our own stuff should be safe (REPORT_JAVASCRIPT and REPORT_CSS).

ebidel · 2017-03-29T21:07:36Z

lighthouse-core/report/v2/report-styles.css

+
+/* Text */
+
+.lighthouse-text {


shadow dom is the solution :)

ebidel · 2017-03-29T21:18:27Z

lighthouse-core/report/v2/report-renderer.js

+   * @param {string=} className
+   * @returns {Element}
+   */
+  _createElement(name, className) {


Shall we support attributes too? There are a couple of places we use them in the current report.

_createElement(name, className, attrs = []) { ... for (const [key, val] of Object.entries(attrs)) { element.setAttribute(key, val); }

Let's do it when we need it?

We can wait if you want. But we'll need it for at least two places:

https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/report/partials/cards.html#L5
https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/report/partials/critical-request-chains.html#L2

I started playing with porting some of the html formatters today and ran into this.

ebidel · 2017-03-29T21:23:52Z

lighthouse-core/report/v2/report-renderer.js

+    const element = this._createElement('div', 'lighthouse-score');
+    const value = this._createChild(element, 'div', 'lighthouse-score-value');
+    value.textContent = score.toLocaleString(undefined, {maximumFractionDigits: 1});
+    if (score <= 50)


fore scoring you can use this (taken from the existing report):

const RATINGS = { GOOD: {label: 'good', minScore: 75}, AVERAGE: {label: 'average', minScore: 45}, POOR: {label: 'poor'} };

Sorry for the double comments. Github was being weird today.

ebidel · 2017-03-29T23:20:03Z

lighthouse-core/report/v2/report-renderer.js

+    const element = this._createElement('div', 'lighthouse-score');
+    const value = this._createChild(element, 'div', 'lighthouse-score__value');
+    value.textContent = score.toLocaleString(undefined, {maximumFractionDigits: 1});
+    if (score <= 50)


For scoring, you can use this from the existing report:

const RATINGS = { GOOD: {label: 'good', minScore: 75}, AVERAGE: {label: 'average', minScore: 45}, POOR: {label: 'poor'} };

ebidel · 2017-03-29T23:21:47Z

lighthouse-core/report/v2/report-renderer.js

+   */
+  _renderScore(score, title, description) {
+    const element = this._createElement('div', 'lighthouse-score');
+    const value = this._createChild(element, 'div', 'lighthouse-score__value');


could easily get away without having a _createChild:

element.appendChild(this._createElement('div', 'lighthouse-score__value'));

_createChild returns an element for further manipulations.

ebidel · 2017-03-29T23:34:41Z

lighthouse-core/report/v2/report-template.html

-    const renderer = new ReportRenderer(window.__LIGHTHOUSE_JSON__);
-    renderer.render(document.body);
+    const renderer = new ReportRenderer(document);
+    document.body.appendChild(renderer.renderReport(window.__LIGHTHOUSE_JSON__));


Hopefully in the near future, we can move some of the UI into static HTML that gets populated. Is there a plan for that? I'm still very concerned about the maintainability and scalability of a pure dom-based approach.

This is against the idea of report being embeddable and template-free.
Regarding maintainability: we plan to implement some basic primitives like list/table/url/etc, so that on average nobody has to touch report generator at all.

I don't think those goals are at odds with a declarative approach. For example, we could declare <template> partials in the page:

<template id="template-audit"> <div class="lighthouse-audit"> <div class="lighthouse-score"> <div class="lighthouse-score__value"></div> <div class="lighthouse-score__text"> <div class="lighthouse-score__title"></div> <div class="lighthouse-score__description"></div> </div> </div> </div> </template>

http://jsbin.com/bojinipecu/edit?html,output

And various embedders could instantiate slightly different <template>s based on their needs. Alternatively, override a render method and add their extra functionality. It's still 100% DOM, but it would be easier to reason about app structure and make modifications to the UI.

This would also lend itself nicely to creating reusable chunks of markup+functionality in the form of web components. Each component would be responsible for its own css/html/js. DevTools could extend a given component if it needs something different.

const categories = __LIGHTHOUSE_JSON__.reportCategories; const container = document.querySelector('lighthouse-category'); categories.forEach(audit => { const auditEl = document.createElement('lighthouse-audit'); auditEl.audit = audit; container.appendChild(auditEl); });

might produce something like:

<lighthouse-category> <lighthouse-audit id="critical-request-chains"> #shadow-root <div class="lighthouse-score">...</div> ... <card-fortmatter></card-fortmatter> </lighthouse-audit> <lighthouse-audit>...</lighthouse-audit> </lighthouse-category>

ebidel · 2017-03-30T05:33:58Z

lighthouse-core/report/v2/report-template.html

  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
  <title>Lighthouse Report</title>
+  <style>%%LIGHTHOUSE_CSS%%</style>


This one causes vscode's syntax highlighting to blow up. WDYT about making this replace str a css comment: <style>/*%%LIGHTHOUSE_CSS%%*/</style>?

paulirish · 2017-04-01T01:19:27Z

There's not too much left here.
@ebidel do you want to finish off this PR? if not, i'm happy to.

ebidel · 2017-04-02T17:56:25Z

Closing down for #1951

Add basic rendering to report generator v2

b785a97

Supports categories, audits, scores, lists, blocks and text. Changed is-on-https audit to show list of urls as an example. Clearly needs some design love.

Fix linter issues

7f34c3f

dgozman commented Mar 29, 2017

View reviewed changes

patrickhulce reviewed Mar 29, 2017

View reviewed changes

paulirish reviewed Mar 29, 2017

View reviewed changes

Adopt BEM and show error stack

27fa3a8

brendankenny reviewed Mar 29, 2017

View reviewed changes

ebidel suggested changes Mar 29, 2017

View reviewed changes

ebidel reviewed Mar 30, 2017

View reviewed changes

paulirish mentioned this pull request Mar 30, 2017

[meta] I/O Burndown #1937

Closed

52 tasks

ebidel mentioned this pull request Apr 1, 2017

Report generator v2 basics (more stuff) #1951

Merged

ebidel closed this Apr 2, 2017


		/* Text */

		.lighthouse-text {


		/* Text */

		.lighthouse-text {

Conversation

dgozman commented Mar 29, 2017

Uh oh!

paulirish commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebidel Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebidel Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebidel Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish commented Apr 1, 2017

brendankenny Mar 29, 2017 •

edited

Loading

paulirish Mar 29, 2017 •

edited

Loading

ebidel Mar 30, 2017 •

edited

Loading

ebidel Mar 30, 2017 •

edited

Loading

ebidel Mar 30, 2017 •

edited

Loading