Report v2: Basic table formatter #2019

paulirish · 2017-04-15T05:22:16Z

It's not very pretty but the basics are there.

adds thumbnail renderer
adds URL renderer (just displays as text for now)
adds table renderer (obviously)
introduces AuditResult.details.itemHeaders for the thead cells

screenshot, lol:

patrickhulce · 2017-04-17T17:44:19Z

lighthouse-core/audits/byte-efficiency/unused-css-rules.js

@@ -179,14 +179,17 @@ class UnusedCSSRules extends Audit {
      return UnusedCSSRules.mapSheetToResult(indexedSheets[sheetId], pageUrl);
    }).filter(sheet => sheet && sheet.wastedBytes > 1024);

+
+    const headings = [
+      {key: 'url', itemType: 'url', text: 'URL'},


we can override this per row correct? it'd be nice to finally have a nice solution for displaying the inline code preview in the url column

to have an individual row change the itemType? that seems a little weird. whyd we want diff itemtypes in the column?

in all the css-based ones the "url column" doesn't always have a url and it's a code snippet preview instead since it's an inline style. it'd be nice to have the details renderer support that too so the code snippets can say {type: "code", content: ".inline-style {...}"} or something

ebidel

Sweet!

ebidel · 2017-04-17T19:30:01Z

lighthouse-core/audits/byte-efficiency/offscreen-images.js

-          totalKb: 'Original',
-          potentialSavings: 'Potential Savings',
-        }
+        headings,


The @return docstring of this method needs to be updated to match.

ebidel · 2017-04-17T19:37:44Z

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

@@ -88,6 +116,33 @@ class DetailsRenderer {
    return element;
  }

+
+  /**
+   * @param {!DetailsRenderer.TableDetailsJSON} details


is TableDetailsJSON typedef'd anywhere? not seeing it.

ebidel · 2017-04-17T19:39:13Z

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

+    const element = this._dom.createElement('img', 'lh-thumbnail');
+    element.src = obj.text.url;
+    // ignore obj.text.mimeType
+    element.alt = 'Image preview';


Should we also add element.title = obj.text.url; so there's a full url when users hover over the image?

ebidel · 2017-04-17T19:40:06Z

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

+   * @return {!Element}
+   */
+  _renderTable(details) {
+    const element = this._dom.createElement('details', 'lh-details', {open: true});


should we open by default? There could be a lot of stuff in the table.

nah. was only for development. will remove. :)

ebidel · 2017-04-17T19:41:22Z

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

+      element.appendChild(this._dom.createElement('summary')).textContent = details.header;
+    }
+
+    const tableElem = element.createChild('table', 'lh-table lh-table__multicolumn');


Do you need lh-table__multicolumn? That was an impl detail of v1's handlebars table.

nope. will nuke.

ebidel · 2017-04-17T19:47:09Z

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

+     * @return {!Element}
+     */
+    self.Element.prototype.createChild = function(elementName, className) {
+      const element = instance.createElement(elementName, className);


pipe through the attrs param too?

i dont think we can because in devtools this method will already exist and it doesnt do attrs

ebidel · 2017-04-17T19:47:37Z

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

+    self.Element.prototype.createChild = function(elementName, className) {
+      const element = instance.createElement(elementName, className);
+      this.appendChild(element);
+      return element;


nit: return this.appendChild(element);

ebidel · 2017-04-17T19:48:39Z

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

+     * @param {string=} className
+     * @return {!Element}
+     */
+    self.Element.prototype.createChild = function(elementName, className) {


If we're comfortable with modifying built in prototypes, then I have a bunch of other helpers to add :)

check out https://github.com/ChromeDevTools/devtools-frontend/blob/master/front_end/dom_extension/DOMExtension.js .. we can polyfill whatever from there we want. (or easier: just load that file in the report)

ebidel · 2017-04-17T19:49:16Z

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

+  --image-preview: 24px;
+}
+
+img.lh-thumbnail {


just .lh-thumbnail? Seems ok to be tag-agnostic.

brendankenny · 2017-04-20T18:59:59Z

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

+     * @param {string=} className
+     * @return {!Element}
+     */
+    self.Element.prototype.createChild = function(elementName, className) {


why not just make this a method on DOM? We add five characters (DOM and a , ) but avoid all the usual downsides of doing this. Also has the benefit that Closure will understand it when not compiled with devtools, we can more easily test it, etc

brendankenny · 2017-04-20T19:01:44Z

lighthouse-core/audits/audit.js

@@ -57,6 +57,60 @@ class Audit {
  }

  /**
+   * @param  {!Audit.Headings} headings
+   * @return {*}


@return {!Array<string>}?

brendankenny · 2017-04-20T19:05:02Z

lighthouse-core/audits/audit.js

+
+  /**
+   * @param  {!Audit.Headings} headings
+   * @param  {!Object} results


hmm, definitely not an object. {!Array<!Object<string, string>>}? It's hard to follow the old createTable helper

brendankenny · 2017-04-20T19:05:58Z

lighthouse-core/audits/audit.js

@@ -57,6 +57,60 @@ class Audit {
  }

  /**
+   * @param  {!Audit.Headings} headings


just one space (here and elsewhere). Is this a vscode setting that needs to be changed?

heh. yah https://github.com/Microsoft/vscode-comment is at fault. reported the bug.

fixed

brendankenny · 2017-04-20T19:07:18Z

lighthouse-core/audits/audit.js

+
+  /**
+   * @param  {!Audit.Headings} headings
+   * @return {!Array<!DetailsRenderer.DetailsJSON>}


this is fine for now, but it won't actually work if we were to turn on type checking on this file. Closure won't be able to see the typedef since it's in another module. We'll have to think of how we want to do that (move report typedefs to their own file?)

brendankenny · 2017-04-20T19:07:33Z

lighthouse-core/audits/audit.js

+
+  /**
+   * @param  {!Audit.Headings} headings
+   * @param  {!Object} results


same type as above results

brendankenny · 2017-04-20T19:08:07Z

lighthouse-core/audits/audit.js

+ */
+Audit.Heading; // eslint-disable-line no-unused-expressions
+
+/** @typedef {!Array<!Audit.Heading>} */


is it worth making a typedef for this?

Actually yeah. Basically we pass around this typedef and never use the Audit.Heading one directly. Not entirely worth it to nuke this guy, i tried.

if that's the case, what about

/** @typedef { * !Array<{ * key: string, * itemType: string, * text: string, * }>} */ Audit.Headings;

and skip Audit.Heading?

brendankenny · 2017-04-20T19:10:42Z

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

+   */
+  _renderThumbnail(obj) {
+    const element = this._dom.createElement('img', 'lh-thumbnail');
+    element.src = obj.text.url;


this isn't a thing on DetailsJSON

brendankenny · 2017-04-20T19:19:02Z

lighthouse-core/audits/audit.js

+    const tableRows = results.map(item => {
+      return headings.map(heading => {
+        return {
+          type: heading.itemType,


why not do this in _renderTable rather than in the JSON report?

shrugz

I'm okay with either.
patrick is fine with either but thinks its slightly better to have all things coming into renderer be our details objects
dgozman also feels the same way, and thinks that if we wanted to conserve the size of the LH object we should shorten type to t etc. ;) ;)

so I'm comfortable expanding the objects over here before we deliver the LHR.

so I'm comfortable expanding the objects over here before we deliver the LHR.

I think with our less rigid details taxonomy now I'm actually more OK with this extra verboseness :)

We can keep an eye on report file size and how much is taken up by "type": "text" and figure out some defaults (default to text or default to header itemType) if/when needed

paulirish

note from sync: default item details type should be text, handled as such in the details-renderer

paulirish · 2017-04-21T03:05:43Z

lighthouse-core/audits/audit.js

@@ -57,6 +57,60 @@ class Audit {
  }

  /**
+   * @param  {!Audit.Headings} headings


heh. yah https://github.com/Microsoft/vscode-comment is at fault. reported the bug.

fixed

ebidel · 2017-05-01T20:52:43Z

@paulirish how's this coming? We've got a lot of audits that need this guy.

paulirish · 2017-05-01T20:53:28Z

@paulirish how's this coming? We've got a lot of audits that need this guy.

roger that. on it.

paulirish · 2017-05-02T16:21:23Z

PTAL. updated.

patrickhulce · 2017-05-02T16:50:31Z

lighthouse-core/audits/audit.js

+    const tableRows = results.map(item => {
+      return headings.map(heading => {
+        const value = item[heading.key];
+        if (typeof value === 'object' && value.type) return value;


patrickhulce · 2017-05-02T16:55:00Z

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

@@ -424,4 +424,14 @@ summary.lh-passed-audits-summary::-webkit-details-marker {
  }
 }

+.lh-table {
+  --image-preview: 24px;


nit: --image-preview-size or something?

ebidel

Couple of things I want to tweak, but this is LGTM to get in.

ebidel · 2017-05-03T04:07:04Z

@brendankenny to you

brendankenny

reviewed!

brendankenny · 2017-05-04T07:28:12Z

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

+ *     type: string,
+ *     header: ({text: string}|undefined),
+ *     items: !Array<!Array<!DetailsRenderer.DetailsJSON>>,
+ *     itemHeaders: !Array<!DetailsRenderer.DetailsJSON>


by letting these be DetailsJSON we're basically descending into recursive madness again since in theory when you call this.render on them they could embed a whole other table (but of course, our css would quickly fall over itself if that sort of thing occurred). This leaves these basically untyped (except our assurance to the compiler that we're correct when we cast in this.render)

I'm not sure of the right solution...maybe we could separate top level rendering (anything currently in this.render) from more primitive-level rendering (just text, url, and thumbnail as of this PR) in a future PR or something, but we would need to make whatever we did non-annoying.

we're basically descending into recursive madness again

are we though?

detailsjson only has type and text.. it can't contain other types.
really it and thumbnail should include the word "item" as they are ALWAYS leaf nodes.

and the render() method should accept all of these leaf & group types. the real bug seems that render's param is typed to a leaf node when we know its not.

detailsjson is the base (structural) type that is the superset of all of them. It only has fields all of them have: type and optional text (really text is there from when they all had text and should probably be removed). render() takes that and casts according to its type to the appropriate type, and we'll take responsibility to make sure that if type is set to x, then that object really is of type x. It's a hack, but simple (we only have to have humans in the loop checking the top level details object), and can easily be reworked once we know the full outlines of the types we need.

Allowing arbitrary nesting means that it's much less simple since we have to guarantee that all the nested items and itemHeaders are really whatever their type says. It also means we're implicitly breaking the no-typedef-cycles, just getting around it by casting, which is what I meant by recursive madness.

If you're saying we should have a different type explicitly for leaf nodes to make this better, I think that's great

ya makes sense. let's sort this out in voice as i have a few ideas.

brendankenny · 2017-05-04T07:30:06Z

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

+   * @return {!Element}
+   */
+  _renderThumbnail(value) {
+    if (/^image/.test(value.mimeType) === false) {


make a note of this in a jsdoc description?

brendankenny · 2017-05-04T07:30:18Z

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

+
+    const element = this._dom.createElement('img', 'lh-thumbnail');
+    element.src = value.url;
+    element.alt = 'Image preview';


best we can do?

was matching our v1. :) i changed it to empty string, which seems better, actually.

brendankenny · 2017-05-04T07:32:15Z

lighthouse-core/audits/byte-efficiency/uses-responsive-images.js

@@ -80,8 +81,7 @@ class UsesResponsiveImages extends ByteEfficiencyAudit {

  /**
   * @param {!Artifacts} artifacts
-   * @return {{results: !Array<Object>, tableHeadings: Object,
-   *     passes: boolean=, debugString: string=}}
+   * @return {{results: !Array<Object>, headings: !Audit.Headings, passes: boolean=, debugString: string=}}


boolean= would be (boolean|undefined), but doesn't look like it can be undefined?
string= would be (string|undefined)

(= only works for @params...technically it means optional param, which just so happens to manifest as being undefined)

thanks. made a new typdef for this guy since he was everywhere.

brendankenny · 2017-05-04T07:32:40Z

lighthouse-core/audits/byte-efficiency/uses-request-compression.js

@@ -46,7 +46,7 @@ class ResponsesAreCompressed extends ByteEfficiencyAudit {
  /**
   * @param {!Artifacts} artifacts
   * @param {number} networkThroughput
-   * @return {!AuditResult}
+   * @return {{results: !Array<Object>, passes: boolean=, headings: !Audit.Headings, debugString: string=}}


brendankenny · 2017-05-04T07:32:48Z

lighthouse-core/audits/byte-efficiency/uses-optimized-images.js

@@ -63,8 +63,7 @@ class UsesOptimizedImages extends ByteEfficiencyAudit {

  /**
   * @param {!Artifacts} artifacts
-   * @return {{results: !Array<Object>, tableHeadings: Object,
-   *     passes: boolean=, debugString: string=}}
+   * @return {{results: !Array<Object>, headings: !Audit.Headings, passes: boolean=, debugString: string=}}


brendankenny · 2017-05-04T07:33:18Z

lighthouse-core/audits/byte-efficiency/offscreen-images.js

@@ -95,8 +96,7 @@ class OffscreenImages extends ByteEfficiencyAudit {

  /**
   * @param {!Artifacts} artifacts
-   * @return {{results: !Array<Object>, tableHeadings: Object,
-   *     passes: boolean=, debugString: string=}}
+   * @return {{results: !Array<Object>, headings: !Audit.Headings, debugString: string=}}


see notes below

brendankenny · 2017-05-04T07:35:54Z

lighthouse-core/audits/audit.js

+ */
+Audit.Heading; // eslint-disable-line no-unused-expressions
+
+/** @typedef {!Array<!Audit.Heading>} */


if that's the case, what about

/** @typedef { * !Array<{ * key: string, * itemType: string, * text: string, * }>} */ Audit.Headings;

and skip Audit.Heading?

brendankenny · 2017-05-04T07:38:34Z

lighthouse-core/audits/audit.js

+    const tableRows = results.map(item => {
+      return headings.map(heading => {
+        const value = item[heading.key];
+        if (typeof value === 'object' && value.type) return value;


results values might not just be a string, then?

paulirish requested a review from ebidel April 15, 2017 05:22

patrickhulce reviewed Apr 17, 2017

View reviewed changes

ebidel suggested changes Apr 17, 2017

View reviewed changes

brendankenny requested changes Apr 20, 2017

View reviewed changes

brendankenny mentioned this pull request Apr 20, 2017

add Closure type checking for report v2 #2043

Merged

paulirish commented Apr 21, 2017

View reviewed changes

paulirish force-pushed the table branch 2 times, most recently from bc73833 to 160e6ef Compare May 1, 2017 23:44

patrickhulce reviewed May 2, 2017

View reviewed changes

ebidel approved these changes May 2, 2017

View reviewed changes

ebidel mentioned this pull request May 3, 2017

Add time to first byte audit #2126

Merged

brendankenny requested changes May 4, 2017

View reviewed changes

ebidel mentioned this pull request May 4, 2017

Add line/code renderer to deprecations audit #2145

Merged

paulirish added 9 commits May 4, 2017 13:36

table basics

97c38dd

optimized images workin.

d7dac3a

move table creation helpers into Audit

a9ad5ca

fix existing tests.

6a3ca90

some jsdoc feedback.

531a460

feedback

6505c69

fix thumbnail rendering.

9a3e79f

fix closure

103fe62

--image-preview-size

b2b18fe

paulirish force-pushed the table branch from dd3efee to bda482b Compare May 4, 2017 20:58

feedback

f12c6a8

paulirish force-pushed the table branch from bda482b to f12c6a8 Compare May 4, 2017 21:13

paulirish merged commit 8652478 into master May 4, 2017

ebidel deleted the table branch May 4, 2017 21:30

paulirish added a commit to paulirish/lighthouse that referenced this pull request May 5, 2017

Report v2: Basic table formatter (GoogleChrome#2019)

3e76e07

Report v2: Basic table formatter #2019

Report v2: Basic table formatter #2019

Conversation

paulirish commented Apr 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel 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

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

brendankenny Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

brendankenny Apr 20, 2017 • edited Loading

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented May 1, 2017

paulirish commented May 1, 2017

paulirish commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel left a comment • edited Loading

Choose a reason for hiding this comment

ebidel commented May 3, 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

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

Choose a reason for hiding this comment

brendankenny Apr 20, 2017 •

edited

Loading

brendankenny Apr 20, 2017 •

edited

Loading

ebidel left a comment •

edited

Loading