report: plugin badge for plugin score gauges #8307

connorjclark · 2019-04-16T04:31:03Z

googlebot · 2019-04-17T17:40:06Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

exterkamp

LGTM

paulirish

just some nits. then good to go.

paulirish · 2019-04-17T22:03:10Z

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

@@ -104,6 +104,10 @@
  --env-name-min-width: 220px;
  --env-tem-padding: 10px 0px;
  --header-padding: 20px 0 20px 0;
+  --plugin-badge-bg: var(--color-white);
+  --plugin-badge-size-big: calc(var(--circle-size-big) / 2.7);


looks like you can drop the -big variant now :)

hmm, can you take another look, b/c this seems necessary to me. the plugin badge is larger for the gauge in the section, so the --plugin-badge-size-big var is derived from the --circle-size-big var

@paulirish ^

ah true.

what i was probably thinking was changing uses of circle-size-big to assign it to circle size:

.lh-category .lh-gauge { - width: var(--circle-size-big); - height: var(--circle-size-big); + --circle-size: var(--circle-size-big); }

and then all your base stuff is derived off the same value. :D

ok but i accidentally did it for #8222

btw this specific example didnt work, i think there's a problem with the data dependency of the variables / scope.

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

paulirish · 2019-04-17T22:04:11Z

lighthouse-core/report/html/templates.html

+      box-shadow: 0 0 4px rgba(0,0,0,.2);
+      border-radius: 25%;
+    }
+    .lh-category .lh-gauge__wrapper--plugin .lh-gauge__svg-wrapper:before {


as mentioned you can remove this now that we're calc() from circle-size

paulirish · 2019-04-17T22:05:30Z

lighthouse-core/report/html/templates.html

-      <circle class="lh-gauge-base" r="56" cx="60" cy="60"></circle>
-      <circle class="lh-gauge-arc" transform="rotate(-90 60 60)" r="56" cx="60" cy="60"></circle>
-    </svg>
+    <!-- Wrapper exists for the ::before plugin icon. Cannot create pseudo-elements on svgs. -->


HTML comments in templates end up in the final report DOM so...

it'd be slightly better to have them on the outide of the (where they are hardly any use.) sucks as I love having these, but they're awkward additions to our shipped DOM.

fuck it lets keep these and remove comment nodes from DOM.cloneTemplate() in a separate PR

fudge it #8390

brendankenny

last things

brendankenny · 2019-04-19T00:09:41Z

lighthouse-core/scripts/build-report-for-autodeployment.js

@@ -14,6 +14,13 @@ const path = require('path');
 const ReportGenerator = require('../../lighthouse-core/report/report-generator.js');
 const lhr = /** @type {LH.Result} */ (require('../../lighthouse-core/test/results/sample_v2.json'));

+lhr.categories.test = {


key on categories should be the same as id. Not sure we actually depend on that anywhere in the report renderer, but just in case...

can you also add a comment (// Add a plugin to demo plugin rendering or whatever) and drop a note in #8159 for the feature request?

brendankenny · 2019-04-19T00:14:03Z

lighthouse-core/report/html/templates.html

@@ -497,6 +497,32 @@
        animation-delay: 250ms;
    }

+    /* The plugin badge overlay */


this comment might make more sense below this, above .lh-gauge__wrapper--plugin, since all category gauges will have an svg-wrapper, they just won't have the :before content

brendankenny · 2019-04-19T00:15:56Z

lighthouse-core/report/html/templates.html

-      <circle class="lh-gauge-base" r="56" cx="60" cy="60"></circle>
-      <circle class="lh-gauge-arc" transform="rotate(-90 60 60)" r="56" cx="60" cy="60"></circle>
-    </svg>
+    <!-- Wrapper exists for the ::before plugin icon. Cannot create pseudo-elements on svgs. -->


should stick to either :before or ::before in comments and in the stylesheet :)

brendankenny · 2019-04-19T00:18:36Z

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

@@ -144,6 +144,17 @@ describe('CategoryRenderer', () => {

    const audits = categoryDOM.querySelectorAll('.lh-audit');
    assert.equal(audits.length, category.auditRefs.length, 'renders correct number of audits');
+
+    assert.equal(categoryDOM.querySelector('.lh-gauge__wrapper--plugin'), null);


a string explanation like the above asserts would be nice (or a comment, but when in Rome, etc). No plugin categories in the sample results.

brendankenny · 2019-04-19T00:19:08Z

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

+  it('plugin category has plugin badge', () => {
+    const category = JSON.parse(
+      JSON.stringify(sampleResults.reportCategories.find(c => c.id === 'seo')));
+    // Any category not of the core


not sure what this comment means :)

Be bloody, bold, and resolute; laugh to scorn The power of core, for none of plugin born Shall harm the test.

i must have left for lunch mid-comment :)

just gonna delete this.

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

brendankenny · 2019-04-19T00:30:58Z

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

+    // Any category not of the core
+    category.id = 'lighthouse-plugin-someplugin';
+    const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
+    assert.ok(categoryDOM.querySelector('.lh-gauge__wrapper--plugin'));


maybe assert slightly more...e.g. there's only one plugin wrapper in the categoryDOM, and pluginWrapper.querySelector('.lh-gauge__label').textContent is the category's title

There's only one category gauge being rendered here, so there's could only be the one wrapper to assert on. I think your advice makes sense only if this test was rendering the whole report?

There's only one category gauge being rendered here, so there's could only be the one wrapper to assert on. I think your advice makes sense only if this test was rendering the whole report?

oh yeah :)

the .lh-gauge__label check still makes sense, though, and that reminds me

we should have another test similar to this but out in report-renderer-test.js, since report-renderer is in charge of rendering the lh-scores-header with all the gauges in it.

brendankenny · 2019-04-19T00:32:02Z

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

@@ -104,6 +104,10 @@
  --env-name-min-width: 220px;
  --env-tem-padding: 10px 0px;
  --header-padding: 20px 0 20px 0;
+  --plugin-badge-bg: var(--color-white);
+  --plugin-badge-size-big: calc(var(--circle-size-big) / 2.7);


@paulirish ^

brendankenny

last test request, but otherwise LGTM!

brendankenny · 2019-04-19T21:13:14Z

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

+    // Any category not of the core
+    category.id = 'lighthouse-plugin-someplugin';
+    const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
+    assert.ok(categoryDOM.querySelector('.lh-gauge__wrapper--plugin'));


There's only one category gauge being rendered here, so there's could only be the one wrapper to assert on. I think your advice makes sense only if this test was rendering the whole report?

oh yeah :)

the .lh-gauge__label check still makes sense, though, and that reminds me

we should have another test similar to this but out in report-renderer-test.js, since report-renderer is in charge of rendering the lh-scores-header with all the gauges in it.

brendankenny · 2019-04-19T23:02:17Z

🧩 📛

connorjclark and others added 27 commits April 9, 2019 17:46

renderer: update gauges

c00d6f1

unsticky for now

3b42a26

score scale

7dd6eb4

tweak score scale

75881d3

tweak gauge outline

80579e9

font size and line height for doc

459afd2

narrow header width

fad00c9

add wrapper to category so that line sep spans width of page

1d9d2ac

better cat wrapper. meow

0fb2be2

fix paddinng in header

86fbabf

Section header text

6db4bee

section header margin

9e68ea5

make score scale round

1846132

fix gauge width

743ed49

fix gauge size and arc

b3da26b

Merge branch 'master' into report-ui-gauges

6ce6bcf

first pass

3a037c3

metric rows

a952a09

slightly better metric/audit coloring

acb2db4

orange

d4137ba

use more new colors

91702bf

add score icon for opps

23f30a1

manual and na

0a8b6ea

remove some unused vars

5cb289d

contrast colors and antialias

f624b71

Merge branch 'master' into report-ui-audit-results

e60a367

report: plugin badge for plugin score gauges

3957a01

connorjclark requested a review from paulirish as a code owner April 16, 2019 04:31

badge on category icon too

5f9e6aa

vercel bot deployed to staging April 16, 2019 04:38 View deployment

move em dash outside description span

0c094a3

exterkamp approved these changes Apr 17, 2019

View reviewed changes

paulirish approved these changes Apr 17, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into report-ui-plugin-gauge

b7e32b2

vercel bot deployed to staging April 18, 2019 06:11 View deployment

connorjclark added 4 commits April 17, 2019 23:38

Merge branch 'master' into report-ui-audit-results

da242e5

square

b642dac

triangle

0dfe4c3

--color-(average, pass, fail)

d9203cc

brendankenny requested changes Apr 19, 2019

View reviewed changes

pr changes

201a0d8

vercel bot deployed to staging April 19, 2019 00:53 View deployment

connorjclark added 2 commits April 18, 2019 17:59

pr changes

357292f

gauge circle size

6b563f5

connorjclark mentioned this pull request Apr 19, 2019

report(redesign): dark theme #8425

Merged

brendankenny approved these changes Apr 19, 2019

View reviewed changes

connorjclark added 4 commits April 19, 2019 14:49

--icon-shape-size

a171abe

--score-shape-margin

dbf1a72

dedupe some -big stuff

c35403a

Merge branch 'report-ui-audit-results' into report-ui-plugin-gauge

deb5ac0

vercel bot deployed to staging April 19, 2019 22:19 View deployment

label test

0328f22

vercel bot deployed to staging April 19, 2019 22:24 View deployment

report render test

c6feb7d

vercel bot deployed to staging April 19, 2019 22:34 View deployment

brendankenny merged commit 49e7a63 into master Apr 19, 2019

brendankenny deleted the report-ui-plugin-gauge branch April 19, 2019 23:02

connorjclark mentioned this pull request Apr 22, 2019

Report redesign for v5 #8185

Closed

55 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: plugin badge for plugin score gauges #8307

report: plugin badge for plugin score gauges #8307

connorjclark commented Apr 16, 2019 •

edited by brendankenny

Loading

googlebot commented Apr 17, 2019

exterkamp left a comment

paulirish left a comment

paulirish Apr 17, 2019

connorjclark Apr 18, 2019

brendankenny Apr 19, 2019

paulirish Apr 19, 2019

connorjclark Apr 19, 2019

connorjclark Apr 19, 2019

paulirish Apr 17, 2019

paulirish Apr 17, 2019

paulirish Apr 17, 2019

connorjclark Apr 18, 2019

brendankenny left a comment

brendankenny Apr 19, 2019

connorjclark Apr 19, 2019

brendankenny Apr 19, 2019

connorjclark Apr 19, 2019

brendankenny Apr 19, 2019

connorjclark Apr 19, 2019

brendankenny Apr 19, 2019

connorjclark Apr 19, 2019

brendankenny Apr 19, 2019 •

edited

Loading

connorjclark Apr 19, 2019

brendankenny Apr 19, 2019

connorjclark Apr 19, 2019

brendankenny Apr 19, 2019

connorjclark Apr 19, 2019

brendankenny Apr 19, 2019

brendankenny left a comment

brendankenny Apr 19, 2019

brendankenny commented Apr 19, 2019

report: plugin badge for plugin score gauges #8307

report: plugin badge for plugin score gauges #8307

Conversation

connorjclark commented Apr 16, 2019 • edited by brendankenny Loading

googlebot commented Apr 17, 2019

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

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 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

brendankenny Apr 19, 2019 • 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

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 19, 2019

connorjclark commented Apr 16, 2019 •

edited by brendankenny

Loading

brendankenny Apr 19, 2019 •

edited

Loading