misc(treemap): first pass#11832
Conversation
patrickhulce
left a comment
There was a problem hiding this comment.
nice!! 🎉
thinking of expanding on the awesome puppeteer test shell you added when split up? :D
|
There's not a lot of UI stuff to test against (that wouldn't simply be testing the treemap library). There should be once more of the UI is added. I added some unit tests. |
| border: solid 1px #666; | ||
| border-radius: 2px; | ||
| /* TODO: what looks better ??? */ | ||
| /* box-sizing: border-box; */ |
There was a problem hiding this comment.
i made a lil snippet script to evaluate how much skew the visualization introduces to the data.
console.clear();
root = $('.webtreemap-node--root');
rootArea = root.getBoundingClientRect().width * root.getBoundingClientRect().height;
totalSkew = 0;
hmm = $$('.webtreemap-node').map(elem=>{
const caption = elem.querySelector('.webtreemap-caption').textContent;
const area = elem.getBoundingClientRect().width * elem.getBoundingClientRect().height;
const areaPct = area / rootArea * 100;
const textPct = caption.match(/\((\d+)%/)[1];
const skew = parseInt(textPct) - areaPct;
totalSkew += Math.abs(skew);
return {
elem,
caption,
skew,
textPct,
areaPct,
};
}
);
console.table(hmm);
console.log('total skew', totalSkew);and adding border-box makes the skew worse, so we probably shouldn't roll with it now.
|
|
||
| initListeners() { | ||
| window.addEventListener('resize', () => { | ||
| this.render(); |
There was a problem hiding this comment.
one of the changes i made is to expose a webtreemap.layout method that'll relayout an existing treemap, instead of rebuilding the entire DOM again.
so i'd recommend doing that for a lil perf win.
(it was substantial with bigass trees)
This also means you wont have to updateColors on a resize.
There was a problem hiding this comment.
done, but still need to update colors, newly added nodes (b/c of bigger viewport) need to be colored too.
|
|
||
| /** @typedef {HTMLElementTagNameMap & {[id: string]: HTMLElement}} HTMLElementByTagName */ | ||
|
|
||
| const KB = 1024; |
There was a problem hiding this comment.
we end up labeling correctly with KiB/MiB, so maybe just rename these to match.
There was a problem hiding this comment.
lol I had to remind myself by reading myself https://hoten.cc/blog/kb-vs-kb/
k
| TreemapUtil.createChildOf(viewModeEl, 'span', 'lh-text-dim').textContent = | ||
| ` (${TreemapUtil.formatBytes(bytes)})`; | ||
|
|
||
| viewModeEl.addEventListener('click', () => { |
There was a problem hiding this comment.
do these function effectively like tabs?
There was a problem hiding this comment.
You'd have to elaborate what features constitute a tab. They are single selection, and selecting one will necessarily re-create the entire treemap DOM.
Should they be a radio input or something?
There was a problem hiding this comment.
just trying to understand how this works. (hard for me since theres only 1 mode currently) in short: yes they are like tabs that change the main content of the page.
radio button semantics are correct, as well... though I don't think we get a lot out of changing it to radios at this point :)
|
(can't figure out how to reply inline to paul's comments on naming refactors ... thx GH) I've been meaning to refactor the naming structure for a while, been punting on that. I'm not sure why I'm hesitating on using I suppose the details of where EDIT: I'm realizing this is basically what you've suggested, but I'm trying to emphasize that I want the treemap app |
paulirish
left a comment
There was a problem hiding this comment.
re: naming and "root" nodes
Let's back up a bit.
We're generating all this data to visualize in a treemap. We create a tree of nodes. Due to the intended UX, we have this hierarchy:
Working from the inside…
- the depth-2 green node represents the effective sourceRoot of a sourcemap (after path separator splitting and collapsing). it's commonly
webpack://. it doesn't exist if there's no sourcemap. its resourceBytes/size are always the same as its red parent. - the depth-1 red node represents a network script. (inline scripts are combined into a single one of these)
- the depth-0 yellow node parents the entire "group" of these network scripts. this node is passed into webtreemap for rendering.
early on in excution, audits/script-treemap-data has no idea the yellow node even exists, so it's ~fine calling the red node a "root node". But over in treemap/main, we have both.
lighthouse/lighthouse-treemap/app/src/main.js
Lines 109 to 110 in 4521501
And that's not only confusing but violates how tree data structures are defined.
we still need some changes here.
I'm having a hard time parsing your comment above for the same reasons. "top node" vs "root node"... 🤔
.....
I'm trying to emphasize that I want the treemap app
renderfunction to work for cases where ascript sourceorfile sourceis the wrong abstraction
This makes sense to me and thanks for pointing it out. its useful to know your goals.
render() already seems satisfied by this requirement and show() doesn't seem so far off either. (more on that in another comment)
.........
I've been trying out some renames.. These feel distinctive and are flexible for the current and future usecases:
currentRootNodeaka D0 yellow node.- Suggesting
currentTreemapRoot. It'll always be what we pass intotreemap.render()so the "treemap" bit identifies well. - TBH i'd also be fine with using "group" in the name: ~currentGroupRoot. But I understand you have future plans to pass specific non-group nodes to
render().
- Suggesting
topNodein script-treemap-data aka D2 green nodes.- how about
sourceRootNode? these are totally specific to sourcemaps so let's name it so? admittedly contains "root" but by coincidence.
- how about
rootNodesin script-treemap-data (currently shown as D1 red nodes)- all this data is specific to "scripts" so let's call them
scriptNodes?
- all this data is specific to "scripts" so let's call them
rootNodesin main.js aka D1 red nodes.- Currently, each one maps to a "script" or "file" (due to inline script merging). I'd be fine with both. But the resourceType breakdown admittedly messes with this as each node will map to a resource type (Stylesheets, XHRs) and contain children which are ~"files".
- Even renaming
currentRootNodeit'd still be massively confusing to keeprooton these. - How about
d1Nodes? Not compelling, but it's clear.primaryNodesorkeyNodesare also decent.blockNodeswas an early okayish idea.
I'm not sure why RootNodeContainer came into being. I collapsed all of that to just Node.
this didn't make sense to me either. nice cleanup. 👍
(A few other responses I tried to place in the code. Hopefully we don't have to fight the Github UX too much.)
| this.render(); | ||
| } | ||
|
|
||
| render() { |
There was a problem hiding this comment.
just spitballing:
it seems reasonable for render() to be passed the currentRootNode arg instead of reading it off the instance.
Also the semantic line between show() and render() isn't obvious and could definitely be shifted. (or the two merged)
i'm thinking ahead to where we want to show different modes.. I could see multiple separate invocations of render()/show().. perhaps it's being passed the array of "root nodes" instead and constructs this wrapper node itself.
| * @param {LH.Artifacts} artifacts | ||
| * @param {LH.Audit.Context} context | ||
| * @return {Promise<TreemapData>} | ||
| * @return {Promise<LH.Treemap.Node[]>} |
There was a problem hiding this comment.
I suppose the details of script-treemap-data could be changed to:
Array<{src: string, node: LH.Treemap.Node}>where node is the "top node" as defined in makeRootNodeFromScriptWithSourceMap (that is, it's identifier isn't the script src, but instead is the top node conceptually from the map's sources file hierarchy, post any collapsing).
This keeps the Node interface general, but would allow the treemap app to display the script src in a way other than as a layer in the node visualization (already possible, but requires assumptions/tree manipulation).
hmmm.. if i understand you right... isn't that what you just had with RootNodeContainer??
TBH I was confused as to why that object shape existed, but..
I agree with you that script-treemap-data currently enforces each script has a node. And I guess in the future there may be some circumstance where you don't want a treemap node for a script. And I see the value from a type perspective from staying more neutral..
However, with both resourcetypes & coverage usecases, we still want script-level nodes. Let's not generalize it too early. I'm +1 for not doing this and keeping script-treemap-data returning Array<LH.Treemap.Node>. This can always change in the future.
| versionJs, | ||
| ...this._resolveSourcesList(this.opts.javascripts), | ||
| ]; | ||
| if (process.env.DEBUG) return contents.join('\n'); |
There was a problem hiding this comment.
aye... i've been adding the undocumented beautify: true in the options which is almost equivalent.
ref #11254
https://lighthouse-h1odnuuh7.vercel.app/gh-pages/treemap/?debug