report: add PWA category badge gauge #6526

brendankenny · 2018-11-12T04:40:26Z

Part of #6395

~~A bit hacked together, but it works. Work in earlier PRs keeps the hackiness fairly well contained, though, and it should be easy to revise what's in this initial draft.~~

Currently

needs tests
the svg needs optimizing (a naive optimization eliminated the gradients, so skipped for now)
there should be a cleaner way to reveal score badges or portions of the score badges
(maybe eventually) needs a rethink of css classes (e.g. should the "gauge" be anything in that spot, or just the gauge-like gauge. If so, what should that spot be called)

brendankenny · 2018-11-12T04:47:52Z

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

  margin: 0 4px;
 }

+.lh-scores-header .lh-gauge--pwa__wrapper {


it would be nice if this could be .lh-gauge__wrapper:last-of-type, but it doesn't work on classes. Open to other ways to do this that don't rely on the pwa wrapper being where it is

should we really care if it's an .lh-gauge?
couldn't we just do .lh-scores-header > :last-child ?

couldn't we just do .lh-scores-header > :last-child ?

we can't because when you don't run the PWA category you don't want a vertical line before the next last-child in line once the PWA gauge is gone :)

Maybe we should make an explicit divider line class, but this seems fine for now (if/when we ever get another special score gauge we can reevaluate)

Of course :) forgot about running without pwa.

patrickhulce

flushing the two comments I had before I had to run out the door, will be back later but screenshots look nice! :D

patrickhulce · 2018-11-12T14:09:40Z

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

+   */
+  renderScoreGauge(category) {
+    // Defer to parent-gauge style if category error.
+    if (category.score === null) {


I can't decide if I like the explicit documentation that score === null when it fails or would prefer to play it safe with Number.isFinite, any insight you used to share with me?

any insight you used to share with me?

I used it just because that's what categoryRenderer.renderScoreGauge uses :)

Looking more into it now, null is what ReportScoring.arithmeticMean returns in case of error, and combined with the fact that we haven't seen any issues with it for plain old categoryRenderer...seems fine?

patrickhulce · 2018-11-12T14:10:02Z

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

+
+    const passingGroupIds = this._getPassingGroupIds(category.auditRefs);
+    let className = 'lh-badged-na';
+    if (passingGroupIds.has('pwa-fast-reliable')) {


to clarify the laughing was on this whole nesting business we need to do, not just this line :)

FWIW, I'd probably find something like

const isReliable = passingGroupIds.has() const isInstallable = ... const isOptimized = ... if (isReliable && isInstallable && isOptimized) className = 'lh-badged-all' else if (isReliable && isInstallable) className = 'lh-badged-reliable-installable' ...

a bit easier to read, but no biggie

ha, that is significantly easier to read. I was very tired when I wrote all that is my only excuse :)

While you were writing this I was just working on a more css-centric version. What do you think of this instead?

The looooong class names are the only downside. Maybe we can short circuit the BEMness to pick short names

patrickhulce

LGTM

didn't realize most of the lines were SVG and I was mostly done already sorry! :)

patrickhulce · 2018-11-13T23:25:49Z

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

+
+    const passingGroupIds = this._getPassingGroupIds(category.auditRefs);
+    let className = 'lh-badged-na';
+    if (passingGroupIds.has('pwa-fast-reliable')) {


to clarify the laughing was on this whole nesting business we need to do, not just this line :)

FWIW, I'd probably find something like

const isReliable = passingGroupIds.has() const isInstallable = ... const isOptimized = ... if (isReliable && isInstallable && isOptimized) className = 'lh-badged-all' else if (isReliable && isInstallable) className = 'lh-badged-reliable-installable' ...

a bit easier to read, but no biggie

patrickhulce · 2018-11-13T23:27:33Z

lighthouse-core/report/html/templates.html

+<template id="tmpl-lh-gauge--pwa">
+  <style>
+    .lh-gauge--pwa {
+      width: calc(3 * var(--header-font-size));


is there significance to this 3 or just feels about right?

is there significance to this 3 or just feels about right?

I copied this over from .lh-gauge since lh-gauge (and its modifiers) had a bunch of svg-score-gauge-specific properties. Basically it just means 60px :)

This is one of the properties that make it seem we need a more general "this is a round-ish thing representing the overall category score" name that includes both the gauge we've had and the badge container (though we could just stick with lh-gauge and move the current gauge stuff down inside).

lh-gauge defines --circle-size: calc(3 * var(--header-font-size)); in its wrapper.

seems good to either do that here too or to move the circle-size into the .lh-vars block.

seems good to either do that here too or to move the circle-size into the .lh-vars block.

after discussion, moved into .lh-vars block. It makes sense that the gauges should stay the same size and it turns out we were using the same number in a few other places that could benefit from a single definition

patrickhulce · 2018-11-13T23:29:44Z

lighthouse-core/report/html/templates.html

+      display: none;
+    }
+
+    .lh-badged-na .lh-gauge--pwa--na {


these are all just lining up the lh-badged-* with lh-gauge--pwa--* right? since the intention is all identical and we would ideally replace something simpler maybe it does make sense to combine them?

hwikyounglee · 2018-11-14T15:06:12Z

💖

Thanks @brendankenny!

I think we can use a few more pixels above the sub group title (to make the total pixel 24px).

💖

paulirish

lgtm % diff handling of the circle-size custom prop

paulirish · 2018-11-16T23:02:22Z

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

+        const categoryGauge = renderer.renderScoreGauge(category);
+
+        // Group gauges that aren't default at the end of the header
+        if (renderer.renderScoreGauge === categoryRenderer.renderScoreGauge) {


hmm! how does equality on a function work? seems fine though i'm curious.

how does equality on a function work?

it's (shared) on the root prototype, so if a renderer descends from CategoryRenderer but doesn't override renderScoreGauge it'll just be the same function object.

I was originally going to do something more complicated with verifying which prototype the function that will be called was defined on, but it doesn't buy anything extra, and considering this is the only special gauge for now, anything much more complicated than a specialized check directly for the pwa renderer would be compounding the silliness.

paulirish · 2018-11-16T23:04:37Z

lighthouse-core/report/html/templates.html

+<template id="tmpl-lh-gauge--pwa">
+  <style>
+    .lh-gauge--pwa {
+      width: calc(3 * var(--header-font-size));


lh-gauge defines --circle-size: calc(3 * var(--header-font-size)); in its wrapper.

seems good to either do that here too or to move the circle-size into the .lh-vars block.

paulirish · 2018-11-16T23:36:07Z

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

+    const tmpl = this.dom.cloneTemplate('#tmpl-lh-gauge--pwa', this.templateContext);
+    const wrapper = /** @type {HTMLAnchorElement} */ (this.dom.find('.lh-gauge--pwa__wrapper',
+      tmpl));
+    wrapper.href = `#${category.id}`;


I'm wondering if we should also clarify the state in a tooltip.

We could do with a full lh-tooltip and bulleted list, but here's a minimal [title] example:

Regardless, I don't think it should hold up this PR. (which is indeed v clean)

yeah, it feels like it should have a tooltip. I'll add it to the list

Alternative idea:
Fast and reliable: 3/3, Installable: 3/3, PWA Optimized: 4/6

report: add PWA category badge gauge

08eaff3

brendankenny requested a review from paulirish as a code owner November 12, 2018 04:40

brendankenny commented Nov 12, 2018

View reviewed changes

patrickhulce reviewed Nov 12, 2018

View reviewed changes

patrickhulce approved these changes Nov 13, 2018

View reviewed changes

use :not() instead

821a74e

brendankenny added 2 commits November 15, 2018 16:34

simplify svg

b738a69

tests

62b01c6

brendankenny mentioned this pull request Nov 16, 2018

PWA category refactor tracking issue #6395

Closed

31 tasks

paulirish mentioned this pull request Nov 16, 2018

Explicit order for report categories #6563

Closed

paulirish approved these changes Nov 16, 2018

View reviewed changes

paulirish reviewed Nov 16, 2018

View reviewed changes

circle-size sharing

0c8a5ff

brendankenny merged commit 7f55977 into master Nov 17, 2018

brendankenny deleted the pwagauge branch November 17, 2018 01:08

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 badge gauge #6526

report: add PWA category badge gauge #6526

brendankenny commented Nov 12, 2018 •

edited

Loading

brendankenny Nov 12, 2018

wardpeet Nov 16, 2018

brendankenny Nov 17, 2018

wardpeet Nov 17, 2018

patrickhulce left a comment

patrickhulce Nov 12, 2018

brendankenny Nov 13, 2018

patrickhulce Nov 12, 2018

patrickhulce Nov 13, 2018

brendankenny Nov 13, 2018

patrickhulce left a comment

patrickhulce Nov 13, 2018

patrickhulce Nov 13, 2018

brendankenny Nov 13, 2018

paulirish Nov 16, 2018

brendankenny Nov 17, 2018

patrickhulce Nov 13, 2018

hwikyounglee commented Nov 14, 2018

paulirish left a comment

paulirish Nov 16, 2018

brendankenny Nov 16, 2018

paulirish Nov 16, 2018

paulirish Nov 16, 2018

brendankenny Nov 16, 2018

hwikyounglee Nov 17, 2018

report: add PWA category badge gauge #6526

report: add PWA category badge gauge #6526

Conversation

brendankenny commented Nov 12, 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

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwikyounglee commented Nov 14, 2018

paulirish 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

brendankenny commented Nov 12, 2018 •

edited

Loading