report: add PSI.prepareLabData() #5804
Conversation
| class PSI { | ||
| /** | ||
| * Returns all the elements that PSI needs to render the report | ||
| * We keep expose this helper method to minimize the 'public' API surface of the renderer |
| * document | ||
| * ); | ||
| * | ||
| * @param {string} LHresponseJsonString |
There was a problem hiding this comment.
- can we call it an
LHRJsonStringor anLHresultJsonString? Or something else. Or do you think this will be the most self explanatory name? - can always add param descriptions to help with that
- the capitalization scheme here is totally crazy. Capital
LH, all lowercaseresponse, camelcaseJson:) - can it just be an
LH.Resultand not a string? (i.e. already parsed)
There was a problem hiding this comment.
name updated to LHResultJsonString
can it just be an LH.Result and not a string? (i.e. already parsed)
I considered that. The fact that it's a string is due to LR. (And me, obv). And at some point we'll fix LR and this won't be a string anymore and will be received on the PSI side as a proper LH.Result (or LHLite.Result)
Since that seems like an implementation detail on the LH/LR side, it seemed reasonable that we eat the maintenance cost. Both LR and third_party/lh-renderer will have to roll in coordination, but that's about it.
There was a problem hiding this comment.
Since that seems like an implementation detail on the LH/LR side, it seemed reasonable that we eat the maintenance cost. Both LR and third_party/lh-renderer will have to roll in coordination, but that's about it.
SG, just wondering
| dom.find('.lh-category-header', perfCategoryEl).remove(); | ||
| // Remove navigation links | ||
| scoreGaugeWrapper.removeAttribute('href'); | ||
| dom.find('.lh-permalink', perfCategoryEl).remove(); |
There was a problem hiding this comment.
these remove()s are definitely the hackiest bits. Could we do with CSS or have a method for it on CategoryRenderer or something else?
There was a problem hiding this comment.
impl-wise it really comes down to avoiding these two lines, so yah we can easily avoid these .remove's.
- option: add extra arg to PerfCatRen.render() that avoids making the permamlink & catHeader elements in the first place. 1 more arg to .render() than the base class, but shrug.
- option: just like
this._dom.isDevTools()we add athis._dom.isPSI()and drop it into PCR.render(). - the CSS solution (
display:none) is possible and technically safe for screenreaders, but it feels a tad messy. - what kinda method on CatRen are you thinking of?
There was a problem hiding this comment.
option: just like
this._dom.isDevTools()we add athis._dom.isPSI()and drop it intoPCR.render().
this has its downsides, but since we have precedent with isDevTools() it seems (to me) like the best option out of these (besides a .isPSI CSS class, but seems like you really don't like that :)
what kinda method on CatRen are you thinking of?
I was hoping if I suggested it vaguely enough you'd assume I meant something in particular and bring that back as a possible solution :)
| describe('DOM', () => { | ||
| let document; | ||
| beforeAll(() => { | ||
| global.URL = URL; // do i need to do this? |
There was a problem hiding this comment.
do i need to do this?
you tell me :P
There was a problem hiding this comment.
do i need to do this?
● DOM › psi prepareLabData helpers › prepareLabData › reports expected data
ReferenceError: URL is not defined
127 | a.target = '_blank';
128 | a.textContent = linkText;
> 129 | a.href = (new URL(linkHref)).href;
| ^
130 | element.appendChild(a);
131 | }
132 | }
at DOM.convertMarkdownLinkSnippets (lighthouse-core/report/html/renderer/dom.js:129:23)
at PerformanceCategoryRenderer._renderMetric (lighthouse-core/report/html/renderer/performance-category-renderer.js:44:40)
at keyMetrics.forEach.item (lighthouse-core/report/html/renderer/performance-category-renderer.js:135:41)
at Array.forEach (<anonymous>)
at PerformanceCategoryRenderer.render (lighthouse-core/report/html/renderer/performance-category-renderer.js:134:16)
at Function.prepareLabData (lighthouse-core/report/html/renderer/psi.js:46:41)
at Object.it (lighthouse-core/test/report/html/renderer/psi-test.js:59:28)
PASS lighthouse-core/test/audits/mainthread-work-breakdown-test.js
apparently yes :)
| }, /no performance category/i); | ||
| }); | ||
|
|
||
| it('throws if there is no perf category', () => { |
| @@ -112,13 +112,20 @@ class PerformanceCategoryRenderer extends CategoryRenderer { | |||
| /** | |||
| * @param {LH.ReportResult.Category} category | |||
| * @param {Object<string, LH.Result.ReportGroup>} groups | |||
| * @param {string=} environment | |||
There was a problem hiding this comment.
maybe be specific about what's accepted...I assume 'PSI'= works but that looks weird :)
| if (!reportLHR.categoryGroups) throw new Error(`No category groups found.`); | ||
|
|
||
| const perfRenderer = new PerformanceCategoryRenderer(dom, new DetailsRenderer(dom)); | ||
| // PSI environment string will assure the categoryHeader and permalink elements are excluded |
and move
getFinalScreenshotfromUtiltoPSIfixes #5803