misc(treemap): root node selector#12360
Conversation
paulirish
left a comment
There was a problem hiding this comment.
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)
| const optionEl = TreemapUtil.createChildOf(bundleSelectorEl, 'option'); | ||
| optionEl.value = String(selectors.length); | ||
| selectors.push(selector); | ||
| optionEl.innerText = text; |
There was a problem hiding this comment.
textContent is cheaper and cleaner
|
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 For example, the unused js audit could link directly to a bundle view using |
|
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 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
left a comment
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| this.render(); | ||
| this.viewModes = this.createViewModes(); |
There was a problem hiding this comment.
doing this inside of setSelector seems odd. why not create viewmodes in constructor?
There was a problem hiding this comment.
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)
paulirish
left a comment
There was a problem hiding this comment.
i have doubts, but we can move this forward and we'll revisit those at the next iteration.
some nits and thoughts below.
| /** @type {LH.Treemap.ViewMode[]} */ | ||
| this.viewModes; | ||
| /** @type {RenderState=} */ | ||
| this.previousRenderState; |
There was a problem hiding this comment.
cant this also be initialized with the same shape? it'd mean you can remove the awkward !this.previousRenderState || bits below, right?
There was a problem hiding this comment.
I did try, but it made things more awkward.
| let node; | ||
| outer: for (const depthOneNodes of Object.values(this.depthOneNodesByGroup)) { | ||
| for (const depthOneNode of depthOneNodes) { | ||
| if (depthOneNode.name === selector.value) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ref #11254