report: consolidate table headers #14315
Conversation
connorjclark
requested review from
brendankenny
and removed request for
a team
brendankenny
left a comment
brendankenny
left a comment
There was a problem hiding this comment.
One request for a prepareReportResult test, but otherwise LGTM. Yay very slow but eventual progress! :)
| * could also be objects with their own type to override this field. | ||
| */ | ||
| itemType: ItemValueType; | ||
| valueType: ItemValueType; |
There was a problem hiding this comment.
complete tangent to this change, but I feel like there was a good reason table rows are called items but I really don't remember it
| */ | ||
| subItemsHeading?: {key: string, valueType?: ItemValueType, displayUnit?: string, granularity?: number}; | ||
|
|
||
| // NOTE: not used by opportunity details, but used in the renderer until table/opportunity unification. |
There was a problem hiding this comment.
What happens with these now? Do opportunities get a default granularity that matches their old ones now that they're tables?
There was a problem hiding this comment.
Looks like nothing changes here.
I don't understand the comment though. comes from #7177 . Is it just saying that no audit that makes an opportunity tables bothers to set granularity/displayUnit? Probably true when written, but now there's at least one (first byte efficiency audit I looked at uses them, duplicated-javascript).
Regardless, same things happens as before with opportunity tables–if granularity/displayUnit is not defined, details-renderer.js various defaults are used instead.
report/renderer/details-renderer.js
Outdated
| * @return {LH.Audit.Details.OpportunityColumnHeading['subItemsHeading']} | ||
| * @return {LH.Audit.Details.TableColumnHeading['subItemsHeading']} | ||
| */ | ||
| _getCanonicalizedsubItemsHeading(subItemsHeading, parentHeading) { |
There was a problem hiding this comment.
this can go too (how important is the falsy key check?)
There was a problem hiding this comment.
ehhh I think only I wanted that line added, and today I don't really see the point. A new audit would surely notice a bad key being referenced during development. so just drop.
3 participants
Fixes #13803
Shouldn't be any surprises here. I pretty much took what @brendankenny did here. Also add some comapt code in
prepareReportResult.