clients(psi): render treemap button #12570
Conversation
|
|
||
| let buttonsEl = metricsEl.querySelector('.lh-buttons'); | ||
| if (!buttonsEl) buttonsEl = this._dom.createChildOf(metricsEl, 'div', 'lh-buttons'); | ||
| const containerEl = opts.container || metricsEl; |
There was a problem hiding this comment.
report-ui-features doesn't have a reference to the root report el, and psi has 2 reports on the page (and not even attached to DOM when installFeatures is called..) so we need a container option to specify where the element should go.
There was a problem hiding this comment.
this would make a great comment on the container: line up above, just sayin' :D
(I stared at the container querySelector being identical to the default for a bit until I scrolled down a smidge further to this comment)
| icon: 'treemap', | ||
| onClick: () => ReportUIFeatures.openTreemap(lhResult, 'url'), | ||
| }); | ||
| } |
There was a problem hiding this comment.
more copy/paste for now.
| // Defined by lib/file-namer.js, but that file does not exist in PSI. PSI doesn't use it, but | ||
| // needs some basic definition so report-ui-features.js typechecks in PSI. | ||
| // @ts-expect-error | ||
| // eslint-disable-next-line no-unused-vars | ||
| function getFilenamePrefix(lhr) { | ||
| } |
There was a problem hiding this comment.
was this from an old version of the change? There's no change in the dependency tree now so this shouldn't be necessary
There was a problem hiding this comment.
I don't follow–report-ui-features calls getFilenamePrefix, which fails the internal BUILD.
We could include the extra file from lib but we don't need it in PSI so meh.
There was a problem hiding this comment.
i think its cuz reportuifeatures wasn't being used in PSI renderer before, but now is.
and closure is whining about the use of this fn from reportuifeatures
There was a problem hiding this comment.
but now is.
ah, yeahh, that's an important bit :)
next renderer roll will add it
There was a problem hiding this comment.
Ah got it, I just assumed tsc. Maybe add "closure compiler" to the comment somewhere? Not what we usually mean by typechecking and I'll probably be confused again the next time I come across this :)
| @@ -81,6 +80,9 @@ function prepareLabData(LHResult, document) { | |||
| const clonedScoreTemplate = dom.cloneTemplate('#tmpl-lh-scorescale', dom.document()); | |||
| const scoreScaleEl = dom.find('.lh-scorescale', clonedScoreTemplate); | |||
|
|
|||
| const reportUIFeatures = new ReportUIFeatures(dom); | |||
There was a problem hiding this comment.
this class's scope is starting to get scary now 😆
my trail of verification that this doesn't do anything we don't want it to
- hm, we need an instance of UIFeatures to
addButtonI wonder why it's not static. - oh it just needs _dom/_document. what else does the constructor do?
- nothing really,
initFeaturesdoes the heavy lifting. - why is
reportUIFeatures.json =needed then? AFAICT it isn't
There was a problem hiding this comment.
this class's scope is starting to get scary now 😆
yes, definitely :) it needs a refactor (e.g. addButton could just be on dom itself or in performanceCategoryRenderer if we really want it scoped just to addMetricsSectionButton), but since I was the one who suggested not doing #12327 before the 8.0 release I figured this was fair and we can fix it up afterwards :)
There was a problem hiding this comment.
- why is
reportUIFeatures.json =needed then? AFAICT it isn't
this seems right
And we could make addButton static now and pass dom in (initFeatures would just pass it along too), but that might take concurrent DevTools changes?
|
|
||
| let buttonsEl = metricsEl.querySelector('.lh-buttons'); | ||
| if (!buttonsEl) buttonsEl = this._dom.createChildOf(metricsEl, 'div', 'lh-buttons'); | ||
| const containerEl = opts.container || metricsEl; |
There was a problem hiding this comment.
this would make a great comment on the container: line up above, just sayin' :D
(I stared at the container querySelector being identical to the default for a bit until I scrolled down a smidge further to this comment)
Co-authored-by: Patrick Hulce <[email protected]>
ref #11254