Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

clients(psi): render treemap button #12570

Merged
merged 7 commits into from
May 27, 2021
Merged

clients(psi): render treemap button #12570

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
30f8cf6
clients(psi): render treemap button
connorjclark May 26, 2021
7b13b4a
fixtest
connorjclark May 27, 2021
4e0de5f
lol
connorjclark May 27, 2021
d2e0275
tsc
connorjclark May 27, 2021
5abdfe6
comment
connorjclark May 27, 2021
02d21df
comment
connorjclark May 27, 2021
5c67661
Update lighthouse-core/report/html/renderer/psi.js
connorjclark May 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions lighthouse-core/report/html/renderer/psi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@
*/
'use strict';

/* globals self DOM PerformanceCategoryRenderer Util I18n DetailsRenderer ElementScreenshotRenderer */

/* globals self DOM PerformanceCategoryRenderer Util I18n DetailsRenderer ElementScreenshotRenderer ReportUIFeatures */

/**
* Returns all the elements that PSI needs to render the report
Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 addButton I wonder why it's not static.
  • oh it just needs _dom/_document. what else does the constructor do?
  • nothing really, initFeatures does the heavy lifting.
  • why is reportUIFeatures.json = needed then? AFAICT it isn't

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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?

reportUIFeatures.json = lhResult;

/** @param {HTMLElement} reportEl */
const installFeatures = (reportEl) => {
if (fullPageScreenshot) {
Expand Down Expand Up @@ -109,6 +111,17 @@ function prepareLabData(LHResult, document) {
ElementScreenshotRenderer.installFullPageScreenshot(
screenshotEl, fullPageScreenshot.screenshot);
}

const showTreemapApp =
lhResult.audits['script-treemap-data'] && lhResult.audits['script-treemap-data'].details;
if (showTreemapApp) {
reportUIFeatures.addButton({
container: reportEl.querySelector('.lh-audit-group--metrics'),
text: Util.i18n.strings.viewTreemapLabel,
icon: 'treemap',
onClick: () => ReportUIFeatures.openTreemap(lhResult, 'url'),
});
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more copy/paste for now.

};

return {scoreGaugeEl, perfCategoryEl, finalScreenshotDataUri, scoreScaleEl, installFeatures};
Expand All @@ -126,6 +139,13 @@ function _getFinalScreenshot(perfCategory) {
return details.data;
}

// 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
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// eslint-disable-next-line no-unused-vars
function getFilenamePrefix(lhr) {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this from an old version of the change? There's no change in the dependency tree now so this shouldn't be necessary

Copy link
Collaborator Author

@connorjclark connorjclark May 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but now is.

ah, yeahh, that's an important bit :)

next renderer roll will add it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)


if (typeof module !== 'undefined' && module.exports) {
module.exports = prepareLabData;
} else {
Expand Down
7 changes: 3 additions & 4 deletions lighthouse-core/report/html/renderer/report-ui-features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,14 @@ class ReportUIFeatures {
}

/**
* @param {{text: string, icon?: string, onClick: () => void}} opts
* @param {{container?: Element, text: string, icon?: string, onClick: () => void}} opts
*/
addButton(opts) {
const metricsEl = this._document.querySelector('.lh-audit-group--metrics');
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// Not supported without metrics group.
const containerEl = opts.container || metricsEl;
Copy link
Collaborator Author

@connorjclark connorjclark May 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if (!metricsEl) return;

let buttonsEl = metricsEl.querySelector('.lh-buttons');
let buttonsEl = containerEl.querySelector('.lh-buttons');
if (!buttonsEl) buttonsEl = this._dom.createChildOf(metricsEl, 'div', 'lh-buttons');

const classes = [
Expand Down Expand Up @@ -551,7 +551,6 @@ class ReportUIFeatures {
* Opens a new tab to the treemap app and sends the JSON results using postMessage.
* @param {LH.Result} json
* @param {'postMessage'|'url'} method
* @protected
*/
static openTreemap(json, method = 'postMessage') {
const treemapData = json.audits['script-treemap-data'].details;
Expand Down
4 changes: 4 additions & 0 deletions lighthouse-core/test/report/html/renderer/psi-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ const CriticalRequestChainRenderer =
require('../../../../report/html/renderer/crc-details-renderer.js');
const ElementScreenshotRenderer =
require('../../../../report/html/renderer/element-screenshot-renderer.js');
const ReportUIFeatures =
require('../../../../report/html/renderer/report-ui-features.js');

const {itIfProtoExists, sampleResultsRoundtripStr} = testUtils.getProtoRoundTrip();
const sampleResultsStr = fs.readFileSync(__dirname + '/../../../results/sample_v2.json', 'utf-8');
Expand Down Expand Up @@ -48,6 +50,7 @@ describe('DOM', () => {
global.PerformanceCategoryRenderer = PerformanceCategoryRenderer;
global.CriticalRequestChainRenderer = CriticalRequestChainRenderer;
global.ElementScreenshotRenderer = ElementScreenshotRenderer;
global.ReportUIFeatures = ReportUIFeatures;

const {window} = new jsdom.JSDOM(TEMPLATE_FILE);
document = window.document;
Expand All @@ -62,6 +65,7 @@ describe('DOM', () => {
global.PerformanceCategoryRenderer = undefined;
global.CriticalRequestChainRenderer = undefined;
global.ElementScreenshotRenderer = undefined;
global.ReportUIFeatures = undefined;
});

describe('psi prepareLabData helpers', () => {
Expand Down