report(details-renderer): support sub-rows within a table #10084

connorjclark · 2019-12-09T22:48:47Z

If a header defines a mutli: { key: string} property, the renderer will render each value in item[key] within the same column of that row. All other properties (besides label - it doesn't apply) can be set for this "multi header" - anything not set defaults to the outer heading.

Note: Originally there was a toplevel multi: {} object, which contained all the keys that could be arrays. Turns out that allowing any property of a audit product item to be an array does not complicate the renderer as much as I thought it would, so I removed it.

Examples

With this PR, nothing yet uses this feature. To see it in use, see the bundle-analysis branch (#10064)

return {
      items,
      headings: [
        {key: 'url', valueType: 'url', multi: {key: 'sources', valueType: 'code'}, label: str_(i18n.UIStrings.columnURL)},
        {key: 'totalBytes', valueType: 'bytes', multi: {key: 'sourceBytes'}, label: str_(i18n.UIStrings.columnSize)},
        {key: 'wastedBytes', valueType: 'bytes', multi: {key: 'sourceWastedBytes'}, label: str_(i18n.UIStrings.columnWastedBytes)},
      ],
    };

__

    /** @type {LH.Audit.Details.OpportunityColumnHeading[]} */
    const headings = [
      {key: 'source', valueType: 'code', multi: {key: 'urls', valueType: 'url'}, label: str_(i18n.UIStrings.columnName)}, // TODO: or 'Source'?
      {key: '_', valueType: 'bytes', multi: {key: 'sourceBytes'}, granularity: 0.05, label: str_(i18n.UIStrings.columnSize)},
      {key: 'wastedBytes', valueType: 'bytes', granularity: 0.05, label: str_(i18n.UIStrings.columnWastedBytes)},
    ];

    return {
      items,
      headings,
      wastedBytesByUrl,
    };

connorjclark · 2019-12-09T22:52:47Z

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

@@ -548,7 +548,7 @@ describe('DetailsRenderer', () => {
        // itemType is overriden by code object
        headings: [{key: 'content', itemType: 'url', text: 'Heading'}],
        items: [
-          {content: {type: 'code', value: 'code object'}},
+          {content: {type: 'code', value: 'https://codeobject.com'}},


The fallback code in the details render will render a URL w/ an invalid url as a code type - so, this test could possible have a false positive (if we ever refactored and broke the fallback type mechanism). This is avoided by using a valid URL as the value.

patrickhulce

pretty straightforward, nice!

🚲 🏠 : WDYT about subitems instead of multi, I had a difficult time at first understanding why the header definition was shaped the way it was until I got to the bit about valueElement and the ChildElement. subitems, children, something along those lines slightly better reflects the relationship IMO

Turns out that allowing any property of a audit product item to be an array does not complicate the renderer as much as I thought it would, so I removed it.

Does it still complicate protobuf story?

patrickhulce · 2019-12-10T02:02:52Z

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

@@ -246,7 +246,7 @@ class DetailsRenderer {
    switch (heading.valueType) {
      case 'bytes': {
        const numValue = Number(value);
-        return this._renderBytes({value: numValue, granularity: 1});


this is unrelated to multi, right? just needed for the eventual js audits?

yeah I noticed it when I was making the js audits. I could pull this out as a one line change. It seems like a mistake to me.

patrickhulce · 2019-12-10T02:04:35Z

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

+      let multi;
+      if (heading.multi) {
+        multi = {
+          key: heading.multi.key,


nit: would be nice to reuse this mapping bit rather than manually map since that's what the original point of the function was, but not big deal

done, but had to disable some ts.

patrickhulce · 2019-12-10T02:06:02Z

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

+   */
+  _renderMultiValue(row, heading) {
+    const values = row[heading.key];
+    if (!Array.isArray(values)) return null;


this is always unexpected, right? any recovery options we have?

this is mostly for ts. perhaps it should console.error something.

I suppose we could wrap a scalar value in an array of one item - but I would rather be strict about what we accept here. If the heading key was misconfigured, the audit developer will surely notice before publishing (otherwise, the data won't ever show!) Same for if the row value is scalar instead of the expected array. Then there's the case where it, by some ungodly bug, is sometimes scalar, sometimes an array - I bet that won't ever happen, so I think we shouldn't bother with a fallback.

patrickhulce · 2019-12-10T02:06:51Z

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

+    if (!Array.isArray(values)) return null;
+    const valueElement = this._dom.createElement('div', 'lh-multi-values');
+    for (const childValue of values) {
+      const childValueElement = this._renderTableValue(childValue, heading);


does this actually work with arbitrary value types or is report css only setup to accept text/url/code/numbers?

I'd expect it to work for any Value types, although I've only used what you listed so far. For code, the default pre browser styles had to be tweaked (way too much whitespace). maybe others would need the same.

connorjclark · 2019-12-10T03:14:29Z

bike house : WDYT about subitems instead of multi, I had a difficult time at first understanding why the header definition was shaped the way it was until I got to the bit about valueElement and the ChildElement. subitems, children, something along those lines slightly better reflects the relationship IMO

subitems might work

Does it still complicate protobuf story?

the details field is an arbitrary struct, so no change on the encoding side. and we don't do anything specific with it in LR, we just pass it to a json serializer.

connorjclark · 2019-12-10T07:58:30Z

how about subheading?

patrickhulce · 2019-12-10T15:05:02Z

how about subheading?

Oh I'd definitely prefer multi over subheading. It's kind of a hack that we're using headings for this at all because it really has nothing to do with what the heading is, headings is really more columnDefinitions at this point. Aren't they just basically extra items that are nested in the same row underneath a different, primary item?

connorjclark · 2019-12-10T17:39:32Z

how about subheading?

Oh I'd definitely prefer multi over subheading. It's kind of a hack that we're using headings for this at all because it really has nothing to do with what the heading is, headings is really more columnDefinitions at this point. Aren't they just basically extra items that are nested in the same row underneath a different, primary item?

ah, that's interesting, to me it seems like a good conceptual fit.

before, each heading defined a single scalar type (heading.itemType). With these changes, it's (heading.itemType, subheading.itemType*). The extra values are displayed vertically so that they look like extra rows, but mentally I consider them to be all one big value in this column's cell. further, all the properties of a heading apply (except for label).

patrickhulce · 2019-12-10T18:29:09Z

Oh, I mean I agree the thing we currently call headings is the perfect fit for this information. I'm just saying it's pretty much a lie that we call it headings because it's defining the behavior of the entire column, not just a label for the heading :) (true before this PR, just moving further down that road)

paulirish · 2019-12-11T22:49:33Z

🚲 🏠 : WDYT about subitems instead of multi

big +1. also multi being a word fragment instead of a noun makes it extra hard.
but i like subitems a lot

and yes at this point it'd make sense to rename headings to columns but.... we'll have to tackle that separately.

paulirish

in general i think this UI treatment provides extra data density that we can definitely use.

i like it.

connorjclark · 2019-12-11T22:59:48Z

and yes at this point it'd make sense to rename headings to columns but.... we'll have to tackle that separately.

that's fine but idk about splitting the terms here at this point. why not keep it consistent (headings / subheadings), and if we ever rename the former we can rename the latter?

patrickhulce · 2019-12-11T23:40:38Z

that's fine but idk about splitting the terms here at this point.

We're not splitting the terms, we use items for the main items and when we define how the subitems are rendered we use subitems :)

The fact you really like subheadings makes me feel like I probably misunderstood how this works 😆

These are multiple values supplied at each individual row, right? AFAICT, we don't have any additional headings as a result of this PR, just additional data being rendered in each row, or is that wrong?

connorjclark · 2019-12-12T00:10:18Z

we use items for the main items and when we define how the subitems are rendered we use subitems

Both the .key and .multi.key access the same item. I feel subitems is misleading.

Paul and I drew some stuff on some paper and came up with a term we both like - subRows.

paulirish · 2019-12-12T00:13:28Z

Paul and I drew some stuff on some paper

we chatted about this a bit offline. i remembered that a design decision of ours was to make items super idiomatic in its design.. basically an ideal object shape for JSON consumers. this is why it's not titled rows.

however headings is all about the visual presentation in the report. a more descriptive name would be columnDefinitions probably.

but here i think we can afford to be quite explicit about how this new property will affect the visual representation. so subRows works for me.

(yes slightly weird that plurals Rows is not an array, but i also think it works)

patrickhulce · 2019-12-12T21:27:43Z

subRows sgtm 👍

patrickhulce

LGTM, what I like about this method of subRows too is that a renderer using the headings but doesn't know about subRows will still be backwards compatible :)

patrickhulce · 2019-12-17T15:48:44Z

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

+  /**
+   * @param {LH.Audit.Details.Value[]} values
+   * @param {LH.Audit.Details.OpportunityColumnHeading} heading
+   * @return {Element?}


doesn't this always return an element? why the ?

it used to not :)

connorjclark added 3 commits December 9, 2019 12:31

initial

2b9babb

rm row.multi

d73c056

tests

3848221

connorjclark requested a review from paulirish as a code owner December 9, 2019 22:48

googlebot added the cla: yes label Dec 9, 2019

delete code not needed yet

774de08

vercel bot deployed to staging December 9, 2019 22:51 View deployment

connorjclark commented Dec 9, 2019

View reviewed changes

fix types

8a5f433

vercel bot deployed to staging December 9, 2019 23:10 View deployment

do not need _ yet

dc75cbf

vercel bot deployed to staging December 9, 2019 23:19 View deployment

patrickhulce reviewed Dec 10, 2019

View reviewed changes

dry _getCanonicalizedHeadingsFromTable

95ab723

vercel bot deployed to staging December 10, 2019 03:39 View deployment

connorjclark mentioned this pull request Dec 11, 2019

core(unused-javascript): augment with source maps #10090

Merged

paulirish reviewed Dec 11, 2019

View reviewed changes

connorjclark added 2 commits December 12, 2019 13:55

subRows

50eb275

move type check

0ee5d5a

vercel bot deployed to staging December 12, 2019 21:57 View deployment

paulirish changed the title ~~report(details-renderer): render multiple values~~ report(details-renderer): support sub-rows within a table Dec 17, 2019

paulirish approved these changes Dec 17, 2019

View reviewed changes

patrickhulce approved these changes Dec 17, 2019

View reviewed changes

ts

878301c

vercel bot deployed to staging December 17, 2019 18:37 View deployment

connorjclark merged commit 4cc48fd into master Dec 17, 2019

connorjclark deleted the report-multi branch December 17, 2019 19:12

patrickhulce mentioned this pull request May 29, 2020

core: subRow refactor, rename to subItem #10867

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report(details-renderer): support sub-rows within a table #10084

report(details-renderer): support sub-rows within a table #10084

connorjclark commented Dec 9, 2019 •

edited

Loading

connorjclark Dec 9, 2019 •

edited

Loading

patrickhulce left a comment

patrickhulce Dec 10, 2019

connorjclark Dec 10, 2019

patrickhulce Dec 10, 2019

connorjclark Dec 10, 2019

patrickhulce Dec 10, 2019

connorjclark Dec 10, 2019

patrickhulce Dec 10, 2019

connorjclark Dec 10, 2019

connorjclark commented Dec 10, 2019

connorjclark commented Dec 10, 2019

patrickhulce commented Dec 10, 2019

connorjclark commented Dec 10, 2019

patrickhulce commented Dec 10, 2019 •

edited

Loading

paulirish commented Dec 11, 2019

paulirish left a comment

connorjclark commented Dec 11, 2019 •

edited

Loading

patrickhulce commented Dec 11, 2019

connorjclark commented Dec 12, 2019

paulirish commented Dec 12, 2019

patrickhulce commented Dec 12, 2019 •

edited

Loading

patrickhulce left a comment

patrickhulce Dec 17, 2019

connorjclark Dec 17, 2019

report(details-renderer): support sub-rows within a table #10084

report(details-renderer): support sub-rows within a table #10084

Conversation

connorjclark commented Dec 9, 2019 • edited Loading

Examples

connorjclark Dec 9, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Dec 10, 2019

connorjclark commented Dec 10, 2019

patrickhulce commented Dec 10, 2019

connorjclark commented Dec 10, 2019

patrickhulce commented Dec 10, 2019 • edited Loading

paulirish commented Dec 11, 2019

paulirish left a comment

Choose a reason for hiding this comment

connorjclark commented Dec 11, 2019 • edited Loading

patrickhulce commented Dec 11, 2019

connorjclark commented Dec 12, 2019

paulirish commented Dec 12, 2019

patrickhulce commented Dec 12, 2019 • edited Loading

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

connorjclark commented Dec 9, 2019 •

edited

Loading

connorjclark Dec 9, 2019 •

edited

Loading

patrickhulce commented Dec 10, 2019 •

edited

Loading

connorjclark commented Dec 11, 2019 •

edited

Loading

patrickhulce commented Dec 12, 2019 •

edited

Loading