report: render sub-rows in their own tr by connorjclark · Pull Request #10553 · GoogleChrome/lighthouse

connorjclark · 2020-04-07T20:36:35Z

Before this change, the sub rows columns didn't align well with each other, except in perfect conditions (short names, wide viewport). This PR makes it so each sub-row is rendered in its own tr element - the table layout does the alignment for us.

narrow:

connorjclark · 2020-04-08T02:45:40Z

weird flake bro

paulirish · 2020-04-13T22:54:25Z

-          if (Array.isArray(values)) {
-            const subRowsElement = this._renderSubRows(values, subRowsHeading);
-            valueFragment.appendChild(subRowsElement);
+      if (!subRowHeadings.some(Boolean)) continue;


did you just reinvent a if (!subRowHeadings.length) conditional? 😎

there is a return null; in the map

paulirish · 2020-04-17T23:17:47Z

    const tbodyElem = this._dom.createChildOf(tableElem, 'tbody');
    for (const row of details.items) {
-      const rowElem = this._dom.createChildOf(tbodyElem, 'tr');
+      // Render the main row.


can you split out into a _renderTableRow() method?

paulirish · 2020-04-17T23:37:25Z

+
+      // All sub-row data arrays should be the same length, but just in case they are not,
+      // determine the longest data array and fill the missing values with an
+      // null pair (which creates an empty cell).


adding nulls to create empty cells seems very weird to me.
its to maintain alignment with the parent headings right? but dont the parent headings already know what their subheadings are? can we use that relationship?

background color only colors the width of the table if there is an element there.

Paul refactor to remove the nulls here: subrow-layout...subrow-layout-by-col

I think most of the complexity in your and my impl. comes from trying to pass the values to this _renderRow function. How about instead the fn uses a callback? I think it'd look like this:

_renderRow(headings, heading => row && row[heading.key]) for i in numSubRow: const subRowEl = _renderRow(headings, heading => row && Array.isArray(row[heading.key]) && row[heading.key][i]) ...

yeah thats a nice solution. :)

Peanut gallery: this seems like it's getting pretty involved for a static, self-generated table json of known width and height being turned into a <table> of known width and height. There's some table cruft we have to live with for legacy support, but subrows are brand new :)

Since (I don't believe) subrows are turned on anywhere yet (and we haven't shipped 6.0 yet regardless), maybe knowing more of the use case now gives a chance to step back and massage the data structure a bit to make dealing with them simpler? (#⁠5 on the classic https://users.ece.utexas.edu/~adnan/pike.html)

paulirish · 2020-04-17T23:41:29Z

  text-decoration: underline dotted #999;
 }

-.lh-sub-rows:not(:first-child) .lh-sub-row {


i kinda miss the previous styling.
i wonder if there's a way we can still have zebra striping on the non-subrows and the subrows feel like they belong.

... trying a few things out it seems impossible without way too much time. ah well.

i kinda miss the previous styling.

I also prefer the previous styling. It's simple if the rendering code adds an "even" or "odd" class to all rows generated from a single data value. Then we just need to duplicate the CSS, one for both even / odd.

It's simple if the rendering code adds an "even" or "odd" class to all rows generated from a single data value.

true! i don't mind that.

Co-Authored-By: Paul Irish <paulirish@google.com>

paulirish · 2020-04-22T00:14:39Z

+        subRowData.push({heading, value: values[i]});
+      }
+
+      const rowEl = this._renderRow(subRowData);


just realized we got a renderTableRow() and a renderRow(). awkward.

maybe renderTableRow => buildTableItemRow ?

paulirish · 2020-04-22T16:05:56Z

+
+      // All sub-row data arrays should be the same length, but just in case they are not,
+      // determine the longest data array and fill the missing values with an
+      // null pair (which creates an empty cell).


yeah thats a nice solution. :)

paulirish · 2020-05-04T21:58:26Z

Awaiting brendan's typecheck magic for subRows. Will enable a new details subrow shape.

paulirish · 2020-05-15T18:57:15Z

This was the draft data shape for subrows we discussed last time.

one subRows array (instead of multiple arrays).

connorjclark · 2020-05-15T19:13:59Z

might also go with subItems: [...]

connorjclark · 2020-05-26T17:30:04Z

@brendankenny do you have an update on the types? If you can't devote any time in the next couple weeks can you possibly share what you've done so far so that I could prioritize it?

brendankenny · 2020-05-28T14:36:45Z

@brendankenny do you have an update on the types? If you can't devote any time in the next couple weeks can you possibly share what you've done so far so that I could prioritize it?

I was hoping to iterate on this more, but lots of things have gotten in the way :) Maybe this is good enough or you can find some ways to improve.

tl;dr: branch with audits and some of the tests converted to try out the proposal: master...subrowstraw

The biggest constraint is what we identified in the beginning: since TableItem has a generic index (so table columns can be named whatever they want to be), all item properties have to have the union of all possible item types as at least their base type (though individual properties can be constrained within that base type).

That means we can't have a unique subRows property with a unique form (if it has that form, every other TableItem property also has to be able to take that form). IIRC, @connorjclark this is what you originally considered and the downside is why you didn't pursue that idea.

The other constraint is for downstream renderers: ideally if a report renderer doesn't support subrows yet, they'll just silently be ignored.

So, possible approaches:

Rather than having a TableItem = {[p: string]: undefined | ItemValue} indexed type, enumerate all possible table item properties. TableItem = {subRows?: TableSubRow[], url?: string, transferSize?: number, location?: SourceLocationValue, ...}. I tried this to see what it would look like and we already have an enormous number of these and would require either reusing existing item property names or adding to audit-details.d.ts every time a new audit table is added. Doable but fairly ridiculous? We really do want it to be an indexed type.
Have a specific TableItems['subRows'] property with aTableSubRow[] type, but have to let any other TableItem property take a TableSubRow[] type, even if they're never that. Feels bad type-wise but can enforce that it's only a subRows property at runtime like we do with a few other aspects of details rendering.
Let only a TableItems['subRows'] take a TableSubRow[], @ts-ignore all the places where this breaks the type system (let's not do this one :)
Current solution on master with downsides discussed above
Other solutions outside of items. e.g. maybe a parallel subItems, but would require bookkeeping of indices between which subItems are connected to which items.

After checking out (1) and not liking it, I tried out (2) and I'm reasonably happy. With this proposal, subrows become a bit more predictable (looking and acting exactly like items), easier to iterate on in the report renderer, and still have the desired property that renderers that don't support subrows yet will just ignore them by default. Downside is looseness of TableSubRows showing up in places we don't want it, but I don't think that's a big issue in practice.

Heart of things:

/** A table item for rows that are nested within a top-level TableItem (row). */
export interface TableSubRows {
  type: 'subrows';
  items: TableItem[];
}

/** Possible types of values found within table items. */
type ItemValue = string | number | boolean | DebugData | NodeValue | SourceLocationValue | LinkValue | UrlValue | CodeValue | TableSubRows;

export interface TableItem {
  debugData?: DebugData;
  subRows?: TableSubRows;
  [p: string]: undefined | ItemValue;
}

and an item with subrows would look something like

{
  "url": "http://example.com/example.js",
  "totalBytes": 71654,
  "wastedBytes": 30470,
  "wastedPercent": 42.52388078488413,
  "subRows": {
    "type": "subrows",
    "items": [{
        "sources": "...Control/assets/js/vendor/katex.min.js",
        "sourceBytes": 14773,
        "sourceWastedBytes": 500,
    }, {
      "//": "..."
    }],
  },
}

report: render sub-rows in their own tr

bb6d7ee

connorjclark requested a review from a team as a code owner April 7, 2020 20:36

connorjclark requested review from paulirish and removed request for a team April 7, 2020 20:36

googlebot added the cla: yes label Apr 7, 2020

devtools-bot assigned paulirish Apr 7, 2020

devtools-bot added the waiting4reviewer label Apr 7, 2020

styles

3bf1bab

vercel Bot deployed to Preview April 7, 2020 20:41 View deployment

connorjclark closed this Apr 8, 2020

connorjclark reopened this Apr 8, 2020

Merge remote-tracking branch 'origin/master' into subrow-layout

e91074b

vercel Bot deployed to Preview April 13, 2020 22:53 View deployment

paulirish requested changes Apr 17, 2020

View reviewed changes

Apply suggestions from code review

1bd9274

Co-Authored-By: Paul Irish <paulirish@google.com>

vercel Bot deployed to Preview April 21, 2020 20:29 View deployment

connorjclark added 4 commits April 21, 2020 13:43

_renderTableRow

7e69d63

delete code

ff6c158

Merge remote-tracking branch 'origin/master' into subrow-layout

b3b0384

zebra

a7eb868

vercel Bot deployed to Preview April 21, 2020 21:05 View deployment

fontsize

687b486

vercel Bot deployed to Preview April 21, 2020 21:08 View deployment

revert

fe2306a

vercel Bot deployed to Preview April 21, 2020 21:10 View deployment

paulirish reviewed Apr 22, 2020

View reviewed changes

paulirish assigned brendankenny May 4, 2020

Merge remote-tracking branch 'origin/master' into subrow-layout

620bac7

vercel Bot deployed to Preview May 15, 2020 21:46 View deployment

connorjclark mentioned this pull request May 29, 2020

core: subRow refactor, rename to subItem #10867

Merged

connorjclark closed this Jun 2, 2020

Conversation

connorjclark commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

connorjclark commented Apr 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark Apr 22, 2020 • edited by paulirish Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark Apr 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulirish Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish commented May 4, 2020

Uh oh!

paulirish commented May 15, 2020

Uh oh!

connorjclark commented May 15, 2020

Uh oh!

connorjclark commented May 26, 2020

Uh oh!

brendankenny commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

connorjclark commented Apr 7, 2020 •

edited

Loading

connorjclark Apr 22, 2020 •

edited by paulirish

Loading

connorjclark Apr 18, 2020 •

edited

Loading

paulirish Apr 22, 2020 •

edited

Loading