Add line/code renderer to deprecations audit #2145

ebidel · 2017-05-04T04:17:10Z

brendankenny

looking good

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

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

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


to avoid circular typedefs, can do header: {type: string, text: (string|undefined)} like ListDetailsJSON does with its items

brendankenny · 2017-05-04T07:17:57Z

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

+    const pre = this._dom.createElement('pre', 'lh-code');
+    pre.textContent = details.text;
+
+    if (details.header) {


header seems like it's overloading the term since it's per-item here, maybe there's something better to call it (based on the screenshot, at least?). I don't have any good suggestions, though

I was using label but decided to keep it consistent with the rest of the details renderers. And it's more or less a "header" for the code snippet.

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

lighthouse-core/audits/deprecations.js

@@ -59,6 +59,19 @@ class Deprecations extends Audit {
          }, log.entry);
        });

+    function makeDepractionsV2(entries) {


jsdoc would be nice here just to not require looking through the impl to see the return type

done. note this does go away when we ditch extendedInfo

ebidel · 2017-05-04T16:21:42Z

PTAL

paulirish · 2017-05-04T16:36:02Z

do we want to group by the entry text? in case there are a bunch of /deep/ errors (or whatever) from various locations?

Also how about visualizing similar to how they're shown in the console, where the location is on the right side:

paulirish · 2017-05-04T16:45:56Z

Combing the two might look something like this:

ebidel · 2017-05-04T17:06:00Z

My understanding is that the deprecation system only warns on first use. So we won't see a bunch of /deep/. Not sure about violations. What pages are you testing against in those screenshots?

ebidel · 2017-05-04T18:11:54Z

Looks like the console now. I also added a warning icon for audit errors.

ebidel · 2017-05-04T20:04:53Z

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

@@ -47,6 +49,16 @@ class DetailsRenderer {
   * @param {!DetailsRenderer.DetailsJSON} text
   * @return {!Element}
   */
+  _renderURL(text) {


stole this from #2019

patrickhulce · 2017-05-04T20:21:34Z

nice! can we have the warning icon not mess with where the text starts and shift it to the left? maybe just beneath the X basically?

ebidel · 2017-05-04T20:29:41Z

@patrickhulce the debug text was aligned with "Open" and this just drops an icon in front of that text. We could align it with the audit X/check but that might be confusing?

I'm ok with ditching it too. Maybe the red it enough .

patrickhulce · 2017-05-04T20:33:54Z

yeah just feels a bit weird, maybe I only actually want the text on the second line to be aligned with the first line instead :)

paulirish · 2017-05-04T20:35:10Z

So we won't see a bunch of /deep/. Not sure about violations. What pages are you testing against in those screenshots?

I was hand-editing those screenshots but it started from the console of https://chromedevtools.github.io/devtools-protocol/

paulirish · 2017-05-04T20:35:47Z

new look generally lgtm. plenty to bikeshed but nothing big.

ebidel · 2017-05-04T20:36:37Z

Here's left aligned. Not sure that it's better. WDYT?

ebidel · 2017-05-04T20:37:27Z

Oh snap. Didn't realize the viewer was Polymer.

patrickhulce · 2017-05-04T20:38:00Z

yeah no the original indent feels a bit better, if we just have the icon stand on its own with some margin before the text sgtm

brendankenny

mostly nits/code style stuff

we're going to need some docs to use some of these renderers :) This one is somewhat non-obvious how each field corresponds to where it ends up in the report

brendankenny · 2017-05-04T20:28:18Z

lighthouse-core/audits/deprecations.js

@@ -59,6 +60,23 @@ class Deprecations extends Audit {
          }, log.entry);
        });

+    /**
+     * @return {!DetailsRenderer.CodeDetailsJSON} details


{!Array<!DetailsRenderer.CodeDetailsJSON>}?

also needs entries

brendankenny · 2017-05-04T20:28:55Z

lighthouse-core/audits/deprecations.js

+    /**
+     * @return {!DetailsRenderer.CodeDetailsJSON} details
+     */
+    function makeDepractionsV2(entries) {


any reason this declaration is nested in audit()? Move it out?

Didn't think it was worth elevating to the class. This func is going away when we ditch extended info.

brendankenny · 2017-05-04T20:31:57Z

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

@@ -36,6 +36,8 @@ class DetailsRenderer {
        return this._renderText(details);
      case 'cards':
        return this._renderCards(/** @type {!DetailsRenderer.CardsDetailsJSON} */ (details));
+      case 'code':


how do you feel about calling this something like codeSnippet? It feels like code will be a more primitive renderer more similar to url or text...just a <pre> and maybe (someday) some syntax highlighting or something

We should land the table renderer so I can use it in this PR :) Then we can make a generic code renderer.

Lolz. This happened while I was responding. BRB!

brendankenny · 2017-05-04T20:39:48Z

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

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


what is text in this case? like a description of the code snippet that follows?

It's the text for the code snippet. For this audit, they're the console message text. We're just that text in <pre> so it looks like DT's log output.

brendankenny · 2017-05-04T20:40:35Z

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

+ *     type: string,
+ *     text: string,
+ *     url: (string|undefined),
+ *     source: string,


what about code instead of source (to differentiate source meaning like the URL or wherever the code is from)

url is where the code came from :) I stuck to source b/c it's what the protocol uses: logEntry.source.

ebidel · 2017-05-04T20:42:25Z

Also:

ebidel · 2017-05-04T20:49:08Z

Non red also looks decent:

ebidel · 2017-05-04T20:50:28Z

LOL. probably should land and tweak later. This stuff is easy to change.

patrickhulce · 2017-05-04T20:58:46Z

ooooh I like this one :) haha yeah it's all good

ebidel · 2017-05-04T22:19:41Z

This is using the table renderer now. Simplifies things a big and renderCode became a standalone renderer for <pre>.

The table render needs some UI love, but we can do that later when the rest of the audits are in that need it.

patrickhulce · 2017-05-04T22:32:38Z

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

-  _renderText(text) {
-    const element = this._dom.createElement('div', 'lh-text');
-    element.textContent = text.text;
+  _renderURL(text) {


ha, any reason to flip these? just adds noise

lost track of which was which :) Github's making it uglier than it actually is.

brendankenny · 2017-05-04T22:44:10Z

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

+ *     lineNumber: (string|undefined)
+ * }}
+ */
+DetailsRenderer.CodeDetailsJSON; // eslint-disable-line no-unused-expressions


delete now? renderCode can just take a DetailsRenderer.DetailsJSON now, I believe

brendankenny

LGTM!

paulirish · 2017-05-04T23:09:36Z

let's do it.

brendankenny requested changes May 4, 2017

View reviewed changes

ebidel commented May 4, 2017

View reviewed changes

brendankenny requested changes May 4, 2017

View reviewed changes

ebidel added 6 commits May 4, 2017 14:30

Add line/code renderer to deprecations audit

5f72722

feedback

bc33e80

Make deprecations look like console output

83651db

comments

07763b1

Remove dupe renderURL after rebase

bcb4528

use table renderer

c749155

ebidel force-pushed the deprecations branch from cf23598 to c749155 Compare May 4, 2017 22:18

patrickhulce reviewed May 4, 2017

View reviewed changes

brendankenny reviewed May 4, 2017

View reviewed changes

delete type def

03fd87c

brendankenny approved these changes May 4, 2017

View reviewed changes

paulirish merged commit d2518c5 into master May 4, 2017

paulirish deleted the deprecations branch May 4, 2017 23:09

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

Add table renderer to deprecations audit (GoogleChrome#2145)

9d71fcf

Add line/code renderer to deprecations audit #2145

Add line/code renderer to deprecations audit #2145

Conversation

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

ebidel commented May 4, 2017

paulirish commented May 4, 2017

paulirish commented May 4, 2017

ebidel commented May 4, 2017 • edited Loading

ebidel commented May 4, 2017

Choose a reason for hiding this comment

patrickhulce commented May 4, 2017

ebidel commented May 4, 2017

patrickhulce commented May 4, 2017

paulirish commented May 4, 2017

paulirish commented May 4, 2017

ebidel commented May 4, 2017 • edited Loading

ebidel commented May 4, 2017

patrickhulce commented May 4, 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

ebidel commented May 4, 2017

ebidel commented May 4, 2017

ebidel commented May 4, 2017

patrickhulce commented May 4, 2017

ebidel commented May 4, 2017

Choose a reason for hiding this comment

ebidel May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented May 4, 2017

ebidel commented May 4, 2017 •

edited

Loading

ebidel commented May 4, 2017 •

edited

Loading

ebidel May 4, 2017 •

edited

Loading