report: add pwa-category-renderer #6486

brendankenny · 2018-11-06T04:37:24Z

part of #6395

adds the basic renderer for the revamped PWA section, with first icons and no longer collapsing behind passing, etc. Right now there are just the grayed out icons for each group (they don't fill in as the score increases).

brendankenny · 2018-11-06T19:40:01Z

@hwikyounglee fixed the icon sizing

patrickhulce

LGTM % nits!

all pretty straight forward stuff, thanks for breaking it up into this first step!

patrickhulce · 2018-11-06T20:36:45Z

lighthouse-core/report/html/renderer/pwa-category-renderer.js

+
+    const auditRefs = category.auditRefs;
+
+    // Regular audits aren't split up into clumps, they're all top-level grouped.


now I'm confused about the difference between a clump and group 😆

~~isn't the highest level collection called a clump now? as in this would be the unexpandable clump of audits?~~

oh wait I think I get it, maybe add into pass/fail/not-applicable clumps as a reminder for ppl like me? :)

maybe add into pass/fail/not-applicable clumps

good idea

patrickhulce · 2018-11-06T20:37:41Z

lighthouse-core/report/html/renderer/pwa-category-renderer.js

+    const auditsElem = this.renderUnexpandableClump(regularAuditRefs, groupDefinitions);
+    categoryElem.appendChild(auditsElem);
+
+    // Manual audits are still clumped.


so overall we have two clumps, regular and manual...right?

yeah, that's confusing. I was thinking of it in terms of there's all the unclumped audits, then the clumped ones (just manual for this time), but it's literally renderUnexpandableClump, so it's just confusing to say they aren't clumped. I'll try to tweak

to add to this confusion, it's renderUnexpandableClump, but we don't add a lh-clump class. We might just need another name for the div holding them (or no name, the same way we don't have a name for any audits that aren't grouped. They're just...there).

patrickhulce · 2018-11-06T20:38:32Z

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

-    const perfCategoryRenderer = new PerformanceCategoryRenderer(this._dom, detailsRenderer);
-    perfCategoryRenderer.setTemplateContext(this._templateContext);
+
+    /** @type {Record<string, CategoryRenderer>} */


hm we should probably gather a type up for our standard category ids soon huh?

hm we should probably gather a type up for our standard category ids soon huh?

yeah, it's hard to know where the boundary should be (the classic question of how much should the config define), but since we were going to hardcode them anyways, this seemed ok for now :) But agreed if we are going to do that, we should just be honest about it

patrickhulce · 2018-11-06T20:40:34Z

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

@@ -92,6 +93,10 @@

  --search-icon-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" width="48" height="48"><path d="M31 28h-1.59l-.55-.55C30.82 25.18 32 22.23 32 19c0-7.18-5.82-13-13-13S6 11.82 6 19s5.82 13 13 13c3.23 0 6.18-1.18 8.45-3.13l.55.55V31l10 9.98L40.98 38 31 28zm-12 0a9 9 0 1 1 .001-18.001A9 9 0 0 1 19 28z" fill="%235E6268"/><path d="M0 0h48v48H0z" fill="none" /></svg>');
  --remove-circle-icon-url: url('data:image/svg+xml;utf8,<svg height="24" width="24" xmlns="http://www.w3.org/2000/svg"><path d="M0 0h24v24H0z" fill="none"/><path d="M7 11v2h10v-2H7zm5-9C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm0 18c-4.41 0-8-3.59-8-8s3.59-8 8-8 8 3.59 8 8-3.59 8-8 8z" fill="%235E6268"/></svg>');
+
+  --pwa-fast-reliable-gray-url: url('data:image/svg+xml;utf8,<svg width="24" height="24" xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="nonzero"><circle fill="#DAE0E3" cx="12" cy="12" r="12"/><path d="M12.3 4l6.3 2.8V11c0 3.88-2.69 7.52-6.3 8.4C8.69 18.52 6 14.89 6 11V6.8L12.3 4zm-.56 12.88l3.3-5.79.04-.08c.05-.1.01-.29-.26-.29h-1.96l.56-3.92h-.56L9.6 12.52c0 .03.07-.12-.03.07-.11.2-.12.37.2.37h1.97l-.56 3.92h.56z" fill="#FFF"/></g></svg>');


I always forget this, but the width="24" height="24" in there is just for relative sizing of itself and our external sizes can scale it without changing the string, right? i.e. it's just a coincidence that the background size of 24px happens to match this right now

(I know we're going to change this URL once it's colored and all that just checking)

So, it looks like the width="24" height="24" does nothing without a viewBox, which svgomg deleted for us. As long as we're happy keeping things at 24px we're good, but if we want to scale up we'll need a viewBox to define the internal coordinate system (which e.g. the <circle fill="#DAE0E3" cx="12" cy="12" r="12"/ is relative to) and then use width and height to control what viewBox is scaled up to.

(although since it's in a background-image, maybe background-size overrides width/height? Or they can be omitted and we can just use background-size? don't know)

patrickhulce · 2018-11-06T20:41:45Z

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

@@ -645,7 +666,10 @@ details, summary {
 .lh-clump > .lh-audit-group__description,
 .lh-audit-group--diagnostics .lh-audit-group__description,
 .lh-audit-group--opportunities .lh-audit-group__description,
-.lh-audit-group--metrics .lh-audit-group__description {
+.lh-audit-group--metrics .lh-audit-group__description,
+.lh-audit-group--pwa-fast-reliable .lh-audit-group__description,


this is getting dicey, maybe we should think about an overall --decorated class or something

this is getting dicey

yeah, agreed. Ideally we can get clumps off of group classes, then we'd just need to update the perf renderer

patrickhulce · 2018-11-06T20:42:19Z

lighthouse-core/test/report/html/renderer/pwa-category-renderer-test.js

+  });
+
+  afterAll(() => {
+    global.URL = undefined;


FWIW, I'm not so sure we need to care about this now with jest

FWIW, I'm not so sure we need to care about this now with jest

won't we run into issues with --runInBand/-i?

I think we've had this in a comment discussion before 😆

AFAIK and have tested, the require/global sandbox is 100% unrelated to whatever processes it decides to run it in

I think we've had this in a comment discussion before 😆

ha, yes. We should actually verify :)

patrickhulce · 2018-11-06T20:44:24Z

lighthouse-core/test/report/html/renderer/report-renderer-test.js

@@ -43,6 +44,11 @@ describe('ReportRenderer', () => {
        require('../../../../report/html/renderer/performance-category-renderer.js');
    }
    global.PerformanceCategoryRenderer = PerformanceCategoryRenderer;
+    if (!PwaCategoryRenderer) {
+      PwaCategoryRenderer =


I'm so confused why we needed to do this for PerformanceCategoryRenderer did you run into something or was it just straight copy?

did you run into something or was it just straight copy?

total copy/paste job. I'll check it out

ah, so the issue is that we require PerformanceCategoryRenderer (same with PwaCategoryRenderer), but it extends CategoryRenderer, assuming CategoryRenderer is a global (as it will be when viewing the report in the browser). So if it's required at the beginning of the test file, it immediately errors since the class declaration is run immediately (other things, like accessing the global URL, aren't run until a method is actually called).

So I think the solution is either something like this, or we wrap the class declaration in something else (which seems worse). Maybe we can come up with a cleaner version of this dance?

well I meant more why there's this weird if and it gets stored in a local instead of getting attached straight to global :) but gotcha 👍

well I meant more why there's this weird if and it gets stored in a local instead of getting attached straight to global

ah, I see now. Yeah. I'll fix in the next PR when I add more tests

exterkamp

Love the new look!!! 🎉

Ultra nit: When clicking a row the icons overlap the 1px blue border.

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

brendankenny · 2018-11-06T22:30:14Z

I feel like a fill of #ffffff is too low contrast

Maybe we can move to a comment in #6395 (or an offshoot issue)?

brendankenny · 2018-11-06T23:01:24Z

Ultra nit: When clicking a row the icons overlap the 1px blue border.

good catch. Normally a group description provides margin space between the group title and the audit results, but the PWA groups have no description. I added a bottom margin to group titles, which fixes the PWA section and collapses when paired with a description, leaving most of the rest of the report unchanged.

It does leave the Metrics title way too high over the metric audits, though, so I overrode the margin just for that one group in the performance section (which seems reasonable, since there's a few lines in the same place entirely devoted to shrinking whitespace in just the metrics group)

brendankenny requested a review from paulirish as a code owner November 6, 2018 04:37

brendankenny mentioned this pull request Nov 6, 2018

PWA category refactor tracking issue #6395

Closed

31 tasks

patrickhulce approved these changes Nov 6, 2018

View reviewed changes

exterkamp reviewed Nov 6, 2018

View reviewed changes

lighthouse-core/report/html/report-styles.css Outdated Show resolved Hide resolved

brendankenny added 6 commits November 6, 2018 14:33

report: add pwa-category-renderer

f79eb99

URL for node 8

9c9c7fb

full size icons

43673d1

feedback

1f9c6e2

update for #6494

ed2d9ea

fix margin

49b5336

brendankenny force-pushed the pwarenderer branch from e64fedf to 49b5336 Compare November 6, 2018 22:57

brendankenny merged commit b832a61 into master Nov 7, 2018

brendankenny deleted the pwarenderer branch November 7, 2018 01:05

brendankenny mentioned this pull request Nov 7, 2018

report(pwa): give badges to groups with all passing audits #6504

Merged

exterkamp mentioned this pull request Feb 15, 2019

PWA in chrome Beta version not reporting score #7253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: add pwa-category-renderer #6486

report: add pwa-category-renderer #6486

brendankenny commented Nov 6, 2018 •

edited

Loading

brendankenny commented Nov 6, 2018

patrickhulce left a comment

patrickhulce Nov 6, 2018

brendankenny Nov 6, 2018

patrickhulce Nov 6, 2018

brendankenny Nov 6, 2018

brendankenny Nov 6, 2018 •

edited

Loading

patrickhulce Nov 6, 2018

brendankenny Nov 6, 2018

patrickhulce Nov 6, 2018

brendankenny Nov 6, 2018

patrickhulce Nov 6, 2018

brendankenny Nov 6, 2018

patrickhulce Nov 6, 2018

brendankenny Nov 6, 2018

patrickhulce Nov 7, 2018

brendankenny Nov 7, 2018

patrickhulce Nov 6, 2018

brendankenny Nov 6, 2018

brendankenny Nov 6, 2018

patrickhulce Nov 7, 2018

brendankenny Nov 7, 2018

exterkamp left a comment

brendankenny commented Nov 6, 2018

brendankenny commented Nov 6, 2018


		const auditRefs = category.auditRefs;

		// Regular audits aren't split up into clumps, they're all top-level grouped.

report: add pwa-category-renderer #6486

report: add pwa-category-renderer #6486

Conversation

brendankenny commented Nov 6, 2018 • edited Loading

brendankenny commented Nov 6, 2018

patrickhulce 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

brendankenny Nov 6, 2018 • 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

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

exterkamp left a comment

Choose a reason for hiding this comment

brendankenny commented Nov 6, 2018

brendankenny commented Nov 6, 2018

brendankenny commented Nov 6, 2018 •

edited

Loading

brendankenny Nov 6, 2018 •

edited

Loading