misc(treemap): root node selector #12360

connorjclark · 2021-04-13T23:25:47Z

paulirish

I spent time time on this and found that we can simplify the data selector idea, both in theory and in code.

Currently it relies on this new selector object, but really all render() needs to know is 1) what's our currentTreemapRoot and 2) what viewMode are we in. So I'm proposing we allow the bundleSelector to set the the first, and the existing viewmode buttons to set the latter. render is then just called in both cases.

Tactically, instead of the array of selectors, we can use a weakmap of type {WeakMap<HTMLOptionElement, LH.Treemap.Node>}. Extrapolating a bit, this means that for the 'All' item, we'll call this.wrapNodesInNewRootNode when building these options (instead of in setViewMode).
Then in the <option> onchange, we can use bundleSelectorEl.selectedOptions[0] to get the selected dom node. we don't call setViewMode because the viewmode didn't change. just set the currentTreeMapRoot and render.

With this in place, we can delete nearly all of setViewMode.

(I do have some working code implementing this idea if you're interested, but i'll leave that to you)

paulirish · 2021-04-16T18:32:11Z

lighthouse-treemap/app/src/main.js

+      const optionEl = TreemapUtil.createChildOf(bundleSelectorEl, 'option');
+      optionEl.value = String(selectors.length);
+      selectors.push(selector);
+      optionEl.innerText = text;


textContent is cheaper and cleaner

lighthouse-treemap/app/src/main.js

connorjclark · 2021-04-16T19:48:31Z

The selector object is meant to be varied by deep-links in the report, while still sending all the treemap data necessary to look at any other view. See Options: eventually viewMode will be added.

For example, the unused js audit could link directly to a bundle view using {lhr: {...}, viewMode: {id: 'unused-bytes', selector: {type: 'depthOneNode', url: '...'}}}}. Treemap app loads to that specific view mode, but user can change viewMode to see any other view. I'm not certain but I wonder if this criteria changes your suggestion.

paulirish · 2021-04-22T17:45:23Z

carrying over some things from our chat for posterity:

so in the case where we deeplink from report to a specific script, we have some options for how it's displayed..

we could basically select it in bundleSelector so its the entire focus AKA setTreemapRoot
or we could still be view all Scripts but visually annotate it AKA annotation
or we can could be viewing All Scripts, but zoom in to it AKA zoom

we were a little skeptical about zoom.. like about treemap area percentages and also the probably awkward UX.

the annotation approach seems most successful for folks repeatedly clicking table items and wanting to see them in the TMViewer..

paulirish

This selector concept currently is just resolving to a Node to set as currentTreemapRoot, but using a string value and string type to refer to it. If we didnt care about these item-level deep links, we could just shortcut things and just only deal with the Nodes that may be set as treemapRoot… I took this approach with a refactor that removed the selector concept.

I think the current separation of treemapRootNode, viewMode aka colorizingMode, and nodesToVisuallyAnnotate makes sense and that's basically the renderState. It makes me think that once we're making deep-linking items happen, then the API could be passing those 3 things, rather than this intermediate selector. But if you're not convinced, sure. Once the deep-linking comes to life I expect we can take another look at how the whole system works.

paulirish · 2021-04-22T17:30:04Z

lighthouse-treemap/app/src/main.js

@@ -256,35 +254,59 @@ class TreemapViewer {
      throw new Error('unknown selector: ' + JSON.stringify(selector));
    }

-    this.render();
+    this.viewModes = this.createViewModes();


doing this inside of setSelector seems odd. why not create viewmodes in constructor?

the labels are based on the bundle selected. also could be that some view modes are not relevant for a selected bundle (like the upcoming duplicated modules)

lighthouse-treemap/app/src/main.js

paulirish

i have doubts, but we can move this forward and we'll revisit those at the next iteration.

some nits and thoughts below.

paulirish · 2021-04-22T18:05:45Z

lighthouse-treemap/app/src/main.js

+    /** @type {LH.Treemap.ViewMode[]} */
+    this.viewModes;
+    /** @type {RenderState=} */
+    this.previousRenderState;


cant this also be initialized with the same shape? it'd mean you can remove the awkward !this.previousRenderState || bits below, right?

I did try, but it made things more awkward.

paulirish · 2021-04-22T18:09:33Z

lighthouse-treemap/app/src/main.js

+      let node;
+      outer: for (const depthOneNodes of Object.values(this.depthOneNodesByGroup)) {
+        for (const depthOneNode of depthOneNodes) {
+          if (depthOneNode.name === selector.value) {


if there any any duplicates, then this can match incorrectly.
with the current data, it seems unlikely, but once we go intra-bundle, i suspect we'll run into this.

this is just depth one nodes, so for a duplicate to be possible that would mean there are multiple instances of network requests with the same URL.

connorjclark added 2 commits April 13, 2021 17:15

misc: treemap selector

85231a1

styles

531c3f0

connorjclark requested a review from a team as a code owner April 13, 2021 23:25

connorjclark requested review from jckr and removed request for a team April 13, 2021 23:25

google-cla bot added the cla: yes label Apr 13, 2021

oops

9857d31

vercel bot deployed to Preview April 13, 2021 23:27 View deployment

devtools-bot assigned jckr Apr 13, 2021

devtools-bot added the waiting4reviewer label Apr 13, 2021

connorjclark assigned paulirish and unassigned jckr Apr 14, 2021

connorjclark requested a review from paulirish April 14, 2021 16:28

paulirish requested changes Apr 16, 2021

View reviewed changes

update

8e60e08

vercel bot deployed to Preview April 17, 2021 00:43 View deployment

paulirish reviewed Apr 22, 2021

View reviewed changes

paulirish approved these changes Apr 22, 2021

View reviewed changes

update

f809afd

vercel bot deployed to Preview April 22, 2021 20:04 View deployment

connorjclark merged commit dd33036 into master Apr 22, 2021

connorjclark deleted the treemap-selector branch April 22, 2021 21:59

connorjclark mentioned this pull request Apr 22, 2021

Lighthouse Treemap ☂️ #11254

Closed

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc(treemap): root node selector #12360

misc(treemap): root node selector #12360

connorjclark commented Apr 13, 2021 •

edited

paulirish left a comment

paulirish Apr 16, 2021

connorjclark commented Apr 16, 2021 •

edited

paulirish commented Apr 22, 2021

paulirish left a comment

paulirish Apr 22, 2021

connorjclark Apr 22, 2021

paulirish left a comment

paulirish Apr 22, 2021

connorjclark Apr 22, 2021

paulirish Apr 22, 2021

connorjclark Apr 22, 2021

misc(treemap): root node selector #12360

misc(treemap): root node selector #12360

Conversation

connorjclark commented Apr 13, 2021 • edited

paulirish left a comment

Choose a reason for hiding this comment

paulirish Apr 16, 2021

Choose a reason for hiding this comment

connorjclark commented Apr 16, 2021 • edited

paulirish commented Apr 22, 2021

paulirish left a comment

Choose a reason for hiding this comment

paulirish Apr 22, 2021

Choose a reason for hiding this comment

connorjclark Apr 22, 2021

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

paulirish Apr 22, 2021

Choose a reason for hiding this comment

connorjclark Apr 22, 2021

Choose a reason for hiding this comment

paulirish Apr 22, 2021

Choose a reason for hiding this comment

connorjclark Apr 22, 2021

Choose a reason for hiding this comment

connorjclark commented Apr 13, 2021 •

edited

connorjclark commented Apr 16, 2021 •

edited