Adjust report-renderer init for devtools #2002

paulirish · 2017-04-13T01:28:58Z

Gettin the new report going in DevTools and wanted some changes like these.

Since I'm subclassing, I need to pass in those subclasses.
Globals weren't being provided automatically. (I think because devtools uses eval?)

e.g. my subclass currently looks like (UPDATED):

Screenshot btw:

brendankenny

Globals weren't being provided automatically

are these loading in devtools as separate files? It's weird that they can't find each other

brendankenny · 2017-04-13T01:56:23Z

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

   */
-  constructor(document) {
-    this._dom = new DOM(document);
+  constructor(document, DOM, DetailsRenderer) {


did you mean to delete this._dom construction?

thx. restored.

patrickhulce · 2017-04-13T16:33:23Z

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

   */
-  constructor(document) {
-    this._dom = new DOM(document);
+  constructor(document, DOM, DetailsRenderer) {


also, should they be passed the constructor or an instance?

probably need to update the tests too

also, should they be passed the constructor or an instance

yeah, I'm in favor of passing in already-constructed instances

patrickhulce · 2017-04-13T16:36:21Z

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

@@ -87,4 +95,6 @@ class DOM {

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


typeof self !== 'undefined' for all of these? I'm ambivalent between that and just having else { } since I can't come up with a case where module and self are both undefined :)

paulirish · 2017-04-14T00:20:56Z

ptal!

ebidel · 2017-04-14T00:18:22Z

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

@@ -56,11 +56,12 @@ function formatNumber(number) {

 class ReportRenderer {
  /**
-   * @param {!Document} document
+   * @param {!DOM} DOM
+   * @param {!DetailsRenderer} DetailsRenderer


lowercase the var names

ebidel · 2017-04-14T00:26:41Z

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

@@ -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);


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

ebidel · 2017-04-14T16:07:23Z

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

@@ -48,7 +49,7 @@ class DOM {
   * @throws {Error}
   */
  cloneTemplate(selector) {


For #2000, I think this should be:

cloneTemplate(selector, context = this.templatesContext) { const template = context.querySelector(selector); ... }

so we can override where to look for subtemplates.

paulirish · 2017-04-14T23:20:27Z

okay @brendankenny @ebidel. all ready for ya?

brendankenny

LGTM. Just some nits

brendankenny · 2017-04-14T23:23:11Z

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

+ *       helpText: string,
+ *       score: (number|boolean),
+ *       scoringMode: string,
+ *       details: (DetailsRenderer.DetailsJSON|DetailsRenderer.CardsDetailsJSON|undefined)


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

brendankenny · 2017-04-14T23:23:21Z

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

+ *     weight: number,
+ *     score: number,
+ *     description: string,
+ *     audits: Array<ReportRenderer.AuditJSON>


!Array<!ReportRenderer.AuditJSON>

brendankenny · 2017-04-14T23:24:41Z

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

+  }
+
+  /**
+   * @param {!Document|!Element} context


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

brendankenny · 2017-04-14T23:25:30Z

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

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


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

brendankenny · 2017-04-14T23:25:44Z

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

+ * @typedef {{
+ *     type: string,
+ *     text: (string|undefined),
+ *     header: (DetailsRenderer.DetailsJSON|undefined),


(!DetailsRenderer.DetailsJSON|undefined)

brendankenny · 2017-04-14T23:25:56Z

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

+/** @typedef {{
+ *     type: string,
+ *     text: string,
+ *     header: DetailsJSON,


!DetailsJSON

brendankenny · 2017-04-14T23:26:17Z

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

+ *     type: string,
+ *     text: string,
+ *     header: DetailsJSON,
+ *     items: Array<{title: string, value: string, snippet: string|undefined, target: string}>


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

mergy purgy

paulirish requested a review from ebidel April 13, 2017 01:28

paulirish force-pushed the rendererinit branch from 2d686a1 to 409331a Compare April 13, 2017 01:30

brendankenny requested changes Apr 13, 2017

View reviewed changes

patrickhulce reviewed Apr 13, 2017

View reviewed changes

paulirish force-pushed the rendererinit branch from 409331a to 111a0c8 Compare April 14, 2017 00:11

brendankenny mentioned this pull request Apr 14, 2017

Report: header, footer, leftnav, share, oh my #2000

Merged

ebidel reviewed Apr 14, 2017

View reviewed changes

ebidel previously requested changes Apr 14, 2017

View reviewed changes

paulirish added 7 commits April 14, 2017 15:58

Adjust report-renderer init for devtools

625f5ba

feedback.

f9033ae

tests.

4ae044a

feedback and align with 2002

96897b9

typedefs on lock. and setTemplateContext

15fe668

tests for cloneTemplate.

70b50cc

tests for renderer.setTemplateContext

0765b35

paulirish force-pushed the rendererinit branch from 8bed784 to 0765b35 Compare April 14, 2017 23:08

brendankenny approved these changes Apr 14, 2017

View reviewed changes

feedback and compile in devtools is good.

46db6a3

paulirish merged commit b12f2ea into master Apr 14, 2017

paulirish deleted the rendererinit branch April 14, 2017 23:56

brendankenny mentioned this pull request Apr 20, 2017

add Closure type checking for report v2 #2043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust report-renderer init for devtools #2002

Adjust report-renderer init for devtools #2002

paulirish commented Apr 13, 2017 •

edited

Loading

brendankenny left a comment

brendankenny Apr 13, 2017

paulirish Apr 14, 2017

patrickhulce Apr 13, 2017

patrickhulce Apr 13, 2017

brendankenny Apr 13, 2017

patrickhulce Apr 13, 2017

paulirish commented Apr 14, 2017

ebidel Apr 14, 2017

ebidel Apr 14, 2017

ebidel Apr 14, 2017 •

edited

Loading

paulirish commented Apr 14, 2017

brendankenny left a comment

brendankenny Apr 14, 2017

brendankenny Apr 14, 2017

brendankenny Apr 14, 2017

brendankenny Apr 14, 2017

brendankenny Apr 14, 2017

brendankenny Apr 14, 2017

brendankenny Apr 14, 2017

Adjust report-renderer init for devtools #2002

Adjust report-renderer init for devtools #2002

Conversation

paulirish commented Apr 13, 2017 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

paulirish commented Apr 14, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Apr 13, 2017 •

edited

Loading

ebidel Apr 14, 2017 •

edited

Loading