Skip to content

misc(treemap): add data table#12363

Merged
devtools-bot merged 18 commits into
masterfrom
treemap-table
Apr 29, 2021
Merged

misc(treemap): add data table#12363
devtools-bot merged 18 commits into
masterfrom
treemap-table

Conversation

@connorjclark

@connorjclark connorjclark commented Apr 14, 2021

Copy link
Copy Markdown
Collaborator

ref #11254

check out the animation in Firefox (Chrome doesn't support animating grid template yet).

deployment: https://lighthouse-git-treemap-table-googlechrome.vercel.app/gh-pages/treemap/?debug

@connorjclark connorjclark requested a review from a team as a code owner April 14, 2021 18:43
@connorjclark connorjclark requested review from adamraine and removed request for a team April 14, 2021 18:43
@google-cla google-cla Bot added the cla: yes label Apr 14, 2021
@connorjclark connorjclark changed the title misc: treemap selector misc(treemap): table Apr 14, 2021
@connorjclark connorjclark changed the base branch from master to treemap-selector April 14, 2021 18:43
@paulirish

Copy link
Copy Markdown
Member

Here's a gif of it in firefox, but in reality the action is a lot smoother than the shitty gif...

Kapture 2021-04-14 at 12 30 51

@connorjclark

Copy link
Copy Markdown
Collaborator Author

(oops, forgot to style the button and place it in the right location)

Comment thread lighthouse-treemap/app/src/main.js Outdated
@brendankenny

Copy link
Copy Markdown
Contributor

is that table library usable piecemeal? It's ~385KiB when used as a whole

@connorjclark

Copy link
Copy Markdown
Collaborator Author

looking into it, there is a "core" bundle in the npm package that is much smaller, but I get an error

image

@connorjclark

connorjclark commented Apr 14, 2021

Copy link
Copy Markdown
Collaborator Author

very nice, they ship all the plugins too. Picking just the ones I used, and the savings is 180KB.

Comment thread lighthouse-treemap/app/src/main.js Outdated
@paulirish

Copy link
Copy Markdown
Member

The interaction i really expect here is something that links the two. As I hover over table items, i'd love to see the corresponding treemap rect get highlighted. And probably same for the inverse.

@connorjclark

Copy link
Copy Markdown
Collaborator Author

yup, but that'll be a followup.

Base automatically changed from treemap-selector to master April 22, 2021 21:59
Comment thread lighthouse-treemap/app/src/main.js Outdated
if (path[0] === this.currentTreemapRoot.name) {
name = path.slice(1).join('/');
} else {
name = path.join('/');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this name creation approach ends up making long names where the important bit is probably at the end.

i don't have a great solution here, but i suspect a lot of the time it's gonna look like this:
image

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I did some stuff already to somewhat alleviate: the // is one, the other is trimming the selected bundle url.

perhaps if the name is a module in a bundle, the entire network URL part should be elided? a tooltip could then provide the url. the followup hover-on-row feature would also serve as a way to link a row to a particular bundle above.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

image

(bundle) or (sm) or something else?

this change shortens things nicely. might still consider eliding long URLs of 3ps..

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

on vc, connor also suggested reusing the color we have above.. so we could do a dot/square using that color. i like that a lot.

the current (bundle) prefix is fine, i don't love it, but whatever.

i really think the tooltip should include the module URL. it makes the tooltip HUGE but i hate when text gets elided because of narrow columns (like in DevTools network panel) and i don't have a quick way of hovering to see what the full thing is.

Comment thread lighthouse-treemap/app/src/main.js Outdated
return TreemapUtil.formatBytes(value);
}},
// eslint-disable-next-line max-len
{title: 'Coverage', field: 'resourceBytes', widthGrow: 3, tooltip: makeCoverageTooltip, formatter: cell => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this column is very wide, and takes away width from the Name column, which definitely needs it more.

IMO this column is just as effective as the same width as the size & unused columns

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I find the column less simple to compare at a smaller width. anyhow I think I can address the large width issue of name to make this moot

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the maxSize tweak we made in the next might change your mind on this, but i'll leave that to you. :)

Comment thread lighthouse-treemap/app/src/main.js Outdated
const gridEl = document.createElement('div');
tableEl.append(gridEl);

const maxSize = this.currentTreemapRoot.resourceBytes;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i see you're already doing some scaling with this maxSize thing, but i think you want the top-most item to be visually 100%, just like the devtools coverage panel viz.

which means this maxSize is probably math.max(...data.map(d => d.resourceBytes))... ish?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

much better! nice catch.

before
image

after
image

my intention here was to maximize the size of these bars (oops, messed that up) and make the sizes relative to each other.

Comment thread lighthouse-treemap/app/src/main.js Outdated
Comment thread lighthouse-treemap/app/src/main.js
Comment thread lighthouse-treemap/app/styles/treemap.css
history: true,
resizableColumns: true,
initialSort: [
{column: 'resourceBytes', dir: 'desc'},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a good initial sort but if I switch to sort by "Unused" or "Size" it's in ascending order first. I have to click again to get the order to be descending.

Is there a way to have descending order be the first option when clicking a column header?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I noticed this too. Notice the arrow for the Coverage column is highlighted. I guess it is selecting the last column that matched the "field" of the initial sort. I only used resourceBytes as the coverage field to sort by that value. I tried your other suggestion of sorting by percent unused, but the results weren't too useful (small modules dominated either end of the sort). Instead I opted to disable sorting for the coverage column.

Is there a way to have descending order be the first option when clicking a column header?

Yes, done.

Comment thread lighthouse-treemap/app/src/main.js Outdated
@brendankenny brendankenny changed the title misc(treemap): table misc(treemap): add data table Apr 23, 2021
@adamraine

Copy link
Copy Markdown
Contributor

Two more ideas:

  • Give the table have a max size so resizing the columns don't push the last column out of view.
  • Add ability to change the table height.

@connorjclark

Copy link
Copy Markdown
Collaborator Author

Give the table have a max size so resizing the columns don't push the last column out of view.

It doesn't really push it out of the viewport. The library doesn't take into account the vertical scrollbar. Unclear how to resolve.

Add ability to change the table height.

This is not supported by CSS grid and implementation via JS is not trivial. I was considering an "export" feature (CSV of the table) which may address the same use case you had in mind. Another option is a fullscreen table button in the future options menu.

@paulirish paulirish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

two lil things

Comment thread lighthouse-treemap/app/src/main.js Outdated
Comment thread lighthouse-treemap/app/styles/treemap.css Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants