core: subRow refactor, rename to subItem by connorjclark · Pull Request #10867 · GoogleChrome/lighthouse

connorjclark · 2020-05-29T01:20:48Z

Continuing from #10553

This is option 2 of @brendankenny's suggested changes to sub-rows, including the changes to the renderer to use it.

googlebot · 2020-05-29T01:20:53Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

connorjclark · 2020-05-29T01:21:28Z

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


Can we just call these subItems at this point?

Can we just call these subItems at this point?

👍 Makes sense to me, not sure if others were involved in the original bike shedding and will want to weigh in

subRows => subItems for the detail type, and subRows => subItemsHeading / subHeading for the headings object?

I think this refactor may have removed some of the hesitation in doing this before?

@patrickhulce @paulirish

you already know I'm onboard for subItems ;)

#10084 (review)

brendankenny · 2020-05-29T01:34:51Z

@googlebot I consent.

googlebot · 2020-05-29T01:35:08Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Beytoven · 2020-06-01T21:05:24Z

-                  '2191 +/- 50',
-                  '2182 +/- 50',
-                  '1259 +/- 50',
+                subItems: [


This is an excellent improvement!

brendankenny

some naming/slight rearranging suggestions but otherwise looking really good to me!

brendankenny · 2020-06-01T21:27:36Z

        /**
         * The name of the property within items being described.
-         * If null, subRows must be defined, and the first sub-row will be empty.
+         * If null, subHeading must be defined, and the first sub-row will be empty.


wasn't this the comment that was confusing before? It's not that the first sub-row will be empty, it's...something else :)

let's do subItem row in this comment.

and sub-item-row in css.

brendankenny · 2020-06-01T21:31:24Z

-            {url: 'https://www.example.com', sources: ['a', 'b', 'c']},
+            {
+              url: 'https://www.example.com',
+              subItems: makeSubitems([


nit: just making this a subitems object is one line longer and doesn't take a trip through a function call :)

I'm trying to reduce object fatigue to increase test readability. The function is free and avoids an extra level of nesting, which for these details objects can be overwhelming.

I'm trying to reduce object fatigue to increase test readability.

the issue is when reading you wonder what makeSubitems() does to the list of things provided and have to go check. Encapsulation often does help, but in this case the transformation is so small (add a type: 'subitems') that the indirection doesn't seem worth it.

But this is minor and not worth haggling over where the mental tradeoff "really" is :)

brendankenny · 2020-06-01T21:43:10Z

      // @ts-ignore: It's ok that there is no text.
-      subRows = this._getCanonicalizedHeading(heading.subRows);
-      if (!subRows.key) {
+      subHeading = this._getCanonicalizedHeading(heading.subHeading);


wdyt about combining the subHeading fixup here and the one down below (where the default values for valueType, granularity, etc are set) into a _getCanonicalizedSubHeading(subHeading, parentHeading) (or whatever) called here or down there?

Puts the subHeading overrides all in one place and makes _getCanonicalizedHeading simpler since it won't need this second execution path within itself

Best I could do was introduce two new functions, one for canonicalizing subHeading and another for deriving a header from a subHeading/parent.

brendankenny · 2020-06-01T21:51:25Z

+  }
+
+  /**
+   * Renders one or more rows from a details table item.


Suggested change

* Renders one or more rows from a details table item.

* Renders one or more rows (if there are subItems) from a details table item.

for some context?

brendankenny · 2020-06-01T21:53:21Z

+    fragment.append(this._renderTableRow(item, headings));
+
+    // A single details item can expand into multiple table rows. These additional table rows
+    // are called sub-rows.


this is a little confusing now to refer to sub-rows but have the property checked immediately below be called subItems. Same with subItems in the LHR but lh-sub-row in the CSS.

Should we commit to the items mistake and go all in on sub-items/lh-sub-items?

I missed updating this. and this comment was more useful here when the function was bigger and more complex. moved it to the function jsdoc.

brendankenny · 2020-06-01T22:12:29Z

+   * @param {LH.Audit.Details.OpportunityItem | LH.Audit.Details.TableItem} item
+   * @param {LH.Audit.Details.OpportunityColumnHeading[]} headings
+   */
+  _renderTableItemRows(item, headings) {


can we come up with some naming options for _renderTableRow and _renderTableItemRows? :)

I assume _renderTableItemRows is saying a table item can be a row or rows, depending on if it had subrows in it? But _renderTableItem also sounds like a singular thing (like _renderTableValue), not a row or rows...

_renderTableRowsFromItem?

what did you think of _renderTableRowsFromItem? Or other permutations?

_renderTableItemRows is kind of gibberish if you're not already in the mindspace of "table items are rows, and each item can result in more than one row if it has subItems"

_renderTableRowsFromItem sgtm!

brendankenny

I'd love to code golf some of the details rendering more, but it might be a lot more productive for me to just make a follow up PR at some poin so you can move on to actually important changes :)

Just the comment and possible method rename left, otherwise LGTM!

brendankenny · 2020-06-02T15:28:29Z

        /**
         * The name of the property within items being described.
-         * If null, subRows must be defined, and the first sub-row will be empty.
+         * If null, subHeading must be defined, and the first subItem row will be empty.


and the first subItem row will be empty

Can this be clarified? I don't remember what we came up with when we discussed what this was saying with @paulirish. There's no empty first subItem rows in any of the tests or expectations...

tried to improve the comment.

writing these details renderer tests is cumbersome... I wish we just diffed HTML snapshots here :/

brendankenny · 2020-06-02T15:37:04Z

-            {url: 'https://www.example.com', sources: ['a', 'b', 'c']},
+            {
+              url: 'https://www.example.com',
+              subItems: makeSubitems([


I'm trying to reduce object fatigue to increase test readability.

the issue is when reading you wonder what makeSubitems() does to the list of things provided and have to go check. Encapsulation often does help, but in this case the transformation is so small (add a type: 'subitems') that the indirection doesn't seem worth it.

But this is minor and not worth haggling over where the mental tradeoff "really" is :)

brendankenny · 2020-06-02T15:39:38Z

+   * @param {LH.Audit.Details.OpportunityItem | LH.Audit.Details.TableItem} item
+   * @param {LH.Audit.Details.OpportunityColumnHeading[]} headings
+   */
+  _renderTableItemRows(item, headings) {


what did you think of _renderTableRowsFromItem? Or other permutations?

_renderTableItemRows is kind of gibberish if you're not already in the mindspace of "table items are rows, and each item can result in more than one row if it has subItems"

connorjclark · 2020-06-04T02:29:40Z

One last thing from me:

Should subHeading be named subItemsHeading?

connorjclark · 2020-06-08T19:26:09Z

oops

brendankenny · 2020-06-08T19:28:43Z

oops?

connorjclark · 2020-06-08T19:32:54Z

I had an outstanding discussion item and didn't realize the merge when green label was here when I fixed the tests. Nbd.

brendankenny and others added 9 commits April 22, 2020 18:02

proposal

536a3fe

update

ee44903

test

8b34c21

core: refactor subrows

2d4fa00

test

88b3f1f

c

9c406f2

sample json

ed9ffb4

tsc

114d0a4

Merge remote-tracking branch 'origin/master' into subrowrefactor

ae3cc31

connorjclark requested a review from a team as a code owner May 29, 2020 01:20

connorjclark requested review from Beytoven and removed request for a team May 29, 2020 01:20

googlebot added the cla: no label May 29, 2020

connorjclark commented May 29, 2020

View reviewed changes

devtools-bot assigned Beytoven May 29, 2020

devtools-bot added the waiting4reviewer label May 29, 2020

googlebot added cla: yes and removed cla: no labels May 29, 2020

subitem

a3180ae

vercel Bot deployed to Preview May 30, 2020 01:27 View deployment

connorjclark changed the title ~~core: sub-row refactor~~ core: subRow refactor, rename to subItem May 30, 2020

Beytoven reviewed Jun 1, 2020

View reviewed changes

Beytoven approved these changes Jun 1, 2020

View reviewed changes

test

49ba52c

vercel Bot deployed to Preview June 1, 2020 21:18 View deployment

brendankenny reviewed Jun 1, 2020

View reviewed changes

details heading refactor

b73ed9c

vercel Bot deployed to Preview June 2, 2020 00:15 View deployment

pr

3bb9119

vercel Bot deployed to Preview June 2, 2020 00:34 View deployment

brendankenny approved these changes Jun 2, 2020

View reviewed changes

_renderTableRowsFromItem

645bd09

vercel Bot deployed to Preview June 2, 2020 18:27 View deployment

comment

0901310

vercel Bot deployed to Preview June 2, 2020 18:34 View deployment

brendankenny added land-when-ci-is-green and removed waiting4reviewer labels Jun 2, 2020

brendankenny reviewed Jun 2, 2020

View reviewed changes

Comment thread lighthouse-cli/test/smokehouse/test-definitions/byte-efficiency/expectations.js Outdated

pr

ef9cdc5

vercel Bot deployed to Preview June 4, 2020 02:29 View deployment

fix

34f48c3

vercel Bot deployed to Preview June 8, 2020 18:27 View deployment

devtools-bot merged commit eb81911 into master Jun 8, 2020

devtools-bot deleted the subrowrefactor branch June 8, 2020 18:52

devtools-bot removed the land-when-ci-is-green label Jun 8, 2020

This was referenced Jun 10, 2020

core(legacy-javascript): reduce polyfills, fix core-js import in test #10937

Merged

report: fix zebra styling, sub item rows for 3p filter #10978

Merged

connorjclark mentioned this pull request Jun 17, 2020

JS analysis blockers ☂️ #10369

Closed

16 tasks

brendankenny mentioned this pull request Jun 25, 2020

v6.1.0 #11025

Merged

	* Renders one or more rows from a details table item.
	* Renders one or more rows (if there are subItems) from a details table item.

Conversation

connorjclark commented May 29, 2020

Uh oh!

googlebot commented May 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark May 29, 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!

brendankenny commented May 29, 2020

Uh oh!

googlebot commented May 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny Jun 1, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brendankenny left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

connorjclark commented Jun 4, 2020

Uh oh!

connorjclark commented Jun 8, 2020

Uh oh!

brendankenny commented Jun 8, 2020

Uh oh!

connorjclark commented Jun 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

connorjclark May 29, 2020 •

edited

Loading

brendankenny Jun 1, 2020 •

edited

Loading