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.

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

misc(treemap): add data table #12363

Merged
merged 18 commits into from
Apr 29, 2021
Merged

misc(treemap): add data table #12363

merged 18 commits into from
Apr 29, 2021

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Apr 14, 2021

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
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
Collaborator Author

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

@brendankenny
Copy link
Member

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

@connorjclark
Copy link
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
Copy link
Collaborator Author

connorjclark commented Apr 14, 2021

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

@paulirish
Copy link
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
Collaborator Author

yup, but that'll be a followup.

Base automatically changed from treemap-selector to master April 22, 2021 21:59
if (path[0] === this.currentTreemapRoot.name) {
name = path.slice(1).join('/');
} else {
name = path.join('/');
Copy link
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
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
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
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.

return TreemapUtil.formatBytes(value);
}},
// eslint-disable-next-line max-len
{title: 'Coverage', field: 'resourceBytes', widthGrow: 3, tooltip: makeCoverageTooltip, formatter: cell => {
Copy link
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
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
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. :)

const gridEl = document.createElement('div');
tableEl.append(gridEl);

const maxSize = this.currentTreemapRoot.resourceBytes;
Copy link
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
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.

lighthouse-treemap/app/src/main.js Outdated Show resolved Hide resolved
lighthouse-treemap/app/src/main.js Show resolved Hide resolved
lighthouse-treemap/app/styles/treemap.css Show resolved Hide resolved
history: true,
resizableColumns: true,
initialSort: [
{column: 'resourceBytes', dir: 'desc'},
Copy link
Member

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
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.

lighthouse-treemap/app/src/main.js Outdated Show resolved Hide resolved
@brendankenny brendankenny changed the title misc(treemap): table misc(treemap): add data table Apr 23, 2021
@adamraine
Copy link
Member

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
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.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

two lil things

lighthouse-treemap/app/src/main.js Outdated Show resolved Hide resolved
lighthouse-treemap/app/styles/treemap.css Outdated Show resolved Hide resolved
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.

None yet

5 participants