report: add device type and version to scorecalc link #10763
Conversation
Co-authored-by: Connor Clark <cjamcl@google.com> Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
| @@ -193,8 +193,8 @@ class ReportRenderer { | |||
| Util.i18n = i18n; | |||
|
|
|||
| const detailsRenderer = new DetailsRenderer(this._dom); | |||
| detailsRenderer.setLighthouseReport(report); | |||
There was a problem hiding this comment.
the perf category renderer and a separate pr #10716 both need some data from the LHR to render, so let's put the report on the details renderer
done via setLighthouseReport so that the property will be optional and existing usages won't break (i.e. progressive enhancement)
There was a problem hiding this comment.
add a comment explaining why we're (somewhat) violating our layering.
There was a problem hiding this comment.
tbh I don't think that's a useful comment.
There was a problem hiding this comment.
it's useful for the same reason that led you to write this review comment. :p
here's what i was thinking
// The scorecalc link (_getScoringCalculatorHref()) and the elementScreenshotRenderer both need data in the LHR outside of what's provided
// With the report stored like this, the property is optional and existing usages won't break.
// TODO: Fix layering in the grand refactor of the ReportRenderer
There was a problem hiding this comment.
// TODO: Fix layering in the grand refactor of the ReportRenderer
@paulirish Please
- Create an issue
- Assign someone
- Assign it a priority
Grand refactors are an engineering smell, but incremental refactors tend to only happen if you do them when you actually need them. Otherwise they need to actually be scheduled.
lighthouse-core/report/html/renderer/performance-category-renderer.js
Outdated
Show resolved
Hide resolved
lighthouse-core/report/html/renderer/performance-category-renderer.js
Outdated
Show resolved
Hide resolved
| @@ -193,8 +193,8 @@ class ReportRenderer { | |||
| Util.i18n = i18n; | |||
|
|
|||
| const detailsRenderer = new DetailsRenderer(this._dom); | |||
| detailsRenderer.setLighthouseReport(report); | |||
There was a problem hiding this comment.
add a comment explaining why we're (somewhat) violating our layering.
…erer.js Co-authored-by: Paul Irish <paulirish@google.com>
| @@ -193,8 +193,8 @@ class ReportRenderer { | |||
| Util.i18n = i18n; | |||
|
|
|||
| const detailsRenderer = new DetailsRenderer(this._dom); | |||
| detailsRenderer.setLighthouseReport(report); | |||
There was a problem hiding this comment.
it's useful for the same reason that led you to write this review comment. :p
here's what i was thinking
// The scorecalc link (_getScoringCalculatorHref()) and the elementScreenshotRenderer both need data in the LHR outside of what's provided
// With the report stored like this, the property is optional and existing usages won't break.
// TODO: Fix layering in the grand refactor of the ReportRenderer
No, I really didn't. I'm saying if you're going to make it a hacked on global, make it like the other hacked on global |
sounds good! |
| * through, we have this global. This property is optional so that existing rendering clients | ||
| * won't break. |
There was a problem hiding this comment.
What existing rendering clients would this not break that won't be broken by the required Util.i18n right below this, also releasing in 6.0?
There was a problem hiding this comment.
I'm thinking anything that uses Category Renderer directly. psi? idk, prob others.
There was a problem hiding this comment.
I'm thinking anything that uses Category Renderer directly. psi? idk, prob others.
Which can roll a new performance-category-renderer and then throw on the first reference to Util.i18n.strings. It's not clear why this clarification is needed when there isn't anyone upgrading with no work.
|
meta comment is this really fly-by-night level urgent to introduce a new global we can't undo until the next major? :) IMO, reports generated during the window between 6.0.0 and 6.0.1 or whatever could live with the score calc eventually being possibly stale if they clicked the link in a year 😉 EDIT: I guess device is pretty important though now that we're fixing the longstanding bug for lrdesktopconfig. Carryon. |
Beat me to it! It would be silly if this was just for the URL longevity via |
follow up to #10754
@paulirish up to you if you wanna merge this into your branch vs as a follow up commit. seems simple enough for the former.