treemap: first pass on Lighthouse Treemap #11545

connorjclark · 2020-10-09T23:47:09Z

This is a first pass of the Lighthouse Treemap app, with minimal features. See https://github.com/GoogleChrome/lighthouse/compare/viewer-treemap-2 for a rough WIP view of what the app might look like with all features intact.

I ripped out a ton of code/features to make a simpler initial PR to review. Please point out if I missed something that could be deferred for later.

Please note that there's quite a bit of design work left to do. I've got the rough look and structure down, but in total it's too much for an initial PR, so let's defer most of it for now.

https://lighthouse-chk0pd53k.vercel.app/treemap/?debug (won't have spacing, waiting on @paulirish to publish webtreemap package)

connorjclark · 2020-10-09T23:47:52Z

lighthouse-core/audits/script-treemap-data.js

@@ -213,7 +213,15 @@ class ScriptTreemapDataAudit extends Audit {
            unusedBytes: unusedJavascriptSummary.sourcesWastedBytes[source],
          };

-          const key = ModuleDuplication.normalizeSource(source);
+          // ModuleDuplication uses keys without the source root prepended, but


Noticed an issue with the duplication stuff. Need to make a separate PR for this.

do you mean split this fix out into a separate PR with tests or there's another fix not shown here that needs to happen in a different PR?

First one. This is the fix.

connorjclark · 2020-10-09T23:50:18Z

lighthouse-core/report/html/renderer/report-ui-features.js

@@ -27,6 +27,9 @@

 /** @typedef {import('./dom')} DOM */

+const VIEWER_ORIGIN = 'http://localhost:8000';


Need a good way to control when to use localhost vs when to use the hosted app. Important for local development. Any ideas?

We could append +dev to our version marker when in local development that unlocks some extra stuff in the report?

connorjclark · 2020-10-09T23:53:38Z

lighthouse-treemap/types/treemap.d.ts

+import _Util = require('../app/src/util.js');
+import {RootNodeContainer as _RootNodeContainer} from '../../lighthouse-core/audits/script-treemap-data';
+import {Node as _Node} from '../../lighthouse-core/audits/script-treemap-data';
+import '../../types/lhr';


This feels wrong. Is it wrong?

connorjclark · 2020-10-09T23:54:23Z

lighthouse-viewer/app/src/lighthouse-report-viewer.js

+      // No UI for treemap yet. For now, must run this command in console.
+      // @ts-ignore
+      globalThis._tmpFeatures = features;
+      const command = '_tmpFeatures._openTreemap();';


Until there is a way to open the treemap viewer in the report, I wanted a way to do in simply through the console.

patrickhulce · 2020-10-13T02:51:16Z

flushing usability notes first (may or may not be solved by upcoming design, just want to make sure they don't get lost when we get to that step if we punt 😃 )

bundles are really hard to distinguish, just kinda looks like one giant treemap (I think the design accounted for this somehow, but this is way more important than I realized)
"All" button in top right has cursor pointer but doesn't seem to do anything yet or have a purpose without the other views, puntable?
the hover border can get hidden sometimes, z-index issue or something maybe?
the rest feels identical to my normal treemaping experience which I'm already very familiar with so I'm probably not the best reviewer to find usability problems there :)

patrickhulce

alright I started reviewing but I think there are still a few subcomponents we can break this up into to prevent a full page screenshots review situation :) I do appreciate being able to look at where it's headed though, very exciting!

taking a quick stab at substeps that might be nice:

creating a basic treemap directory and page that dumps an LHR into a <pre> tag with a test (can discuss the build process, deployment, organization, etc)
adding in the report-ui-features logic to open an LHR from the report (can discuss the origin customization issues, commentary there)
adding in the main treemap viewer logic to the page

each of those chunks feel like they have quite a bit to discuss on their own, WDYT?

patrickhulce · 2020-10-13T02:53:01Z

lighthouse-core/audits/script-treemap-data.js

@@ -213,7 +213,15 @@ class ScriptTreemapDataAudit extends Audit {
            unusedBytes: unusedJavascriptSummary.sourcesWastedBytes[source],
          };

-          const key = ModuleDuplication.normalizeSource(source);
+          // ModuleDuplication uses keys without the source root prepended, but


do you mean split this fix out into a separate PR with tests or there's another fix not shown here that needs to happen in a different PR?

patrickhulce · 2020-10-13T02:55:56Z

lighthouse-core/report/html/renderer/report-ui-features.js

@@ -27,6 +27,9 @@

 /** @typedef {import('./dom')} DOM */

+const VIEWER_ORIGIN = 'http://localhost:8000';


We could append +dev to our version marker when in local development that unlocks some extra stuff in the report?

patrickhulce · 2020-10-13T02:56:23Z

lighthouse-core/report/html/renderer/report-ui-features.js

@@ -27,6 +27,9 @@

 /** @typedef {import('./dom')} DOM */

+const VIEWER_ORIGIN = 'http://localhost:8000';
+const TREEMAP_URL = `${VIEWER_ORIGIN}/treemap/`;


this is only when local though right? when deployed won't it be ${VIEWER_ORIGIN}/lighthouse/treemap/?

seems like the path and origin will be environment dependent

patrickhulce · 2020-10-13T02:58:07Z

lighthouse-core/report/html/renderer/report-ui-features.js

+  /**
+   * Opens a new tab to an external page and sends data using postMessage.
+   * @param {Object} data
+   * @param {string} path


Suggested change

* @param {string} path

* @param {string} url

patrickhulce · 2020-10-13T03:02:15Z

lighthouse-core/report/html/renderer/report-ui-features.js

+
+  /**
+   * Opens a new tab to an external page and sends data using postMessage.
+   * @param {Object} data


Suggested change

* @param {Object} data

* @template {{lhresult: LH.Result}} T

* @param {T} data

(we should probably update viewer to accept lhr though 😄 )

lhresult?

Planning to add a mode property to the data sent to treemap app. At that point, there's no overlap in types, hence object. We don't do nothing but serialize this value, so it seems fine to me.

lhresult?

Isn't that what viewer uses?

At that point, there's no overlap in types, hence object.

They both require an LHR, there could be some overlap in types if we wanted to.

We don't do nothing but serialize this value, so it seems fine to me.

I'm separately advocating elsewhere in this PR that we do do something with it ;)

patrickhulce · 2020-10-13T03:11:09Z

lighthouse-treemap/README.md

+# in separate terminal, start build watch
+find lighthouse-treemap | entr -s 'DEBUG=1 yarn build-treemap'
+
+# visit http://localhost:8000/treemap/?debug


Suggested change

# visit http://localhost:8000/treemap/?debug

open http://localhost:8000/treemap/?debug

:)

patrickhulce · 2020-10-13T03:12:04Z

lighthouse-treemap/README.md

+yarn serve-treemap
+
+# in separate terminal, start build watch
+find lighthouse-treemap | entr -s 'DEBUG=1 yarn build-treemap'


is this a new dependency we need to add to CONTRIBUTING.md?

$ which entr entr not found

No, that doesn't seem important to add there. I'll just add a comment here brew install entr.

build/build-treemap.js

patrickhulce · 2020-10-13T03:25:31Z

lighthouse-treemap/app/index.html

+            <span class="lh-header--url"></span> • <span class="lh-header--size lh-text-dim"></span></span>
+          </span>
+          <div class="panel panel--modals">
+            <!-- View modes go here -->


maybe use an id for component slots?

patrickhulce · 2020-10-13T03:39:54Z

lighthouse-treemap/app/src/main.js

+  }
+
+  render() {
+    webtreemap.render(this.el, this.currentRootNode, {


I guess we own webtreemap, so you're confident we can customize this as much as we need for our purposes?

connorjclark · 2020-10-13T04:51:52Z

flushing usability notes first (may or may not be solved by upcoming design, just want to make sure they don't get lost when we get to that step if we punt 😃 )

bundles are really hard to distinguish, just kinda looks like one giant treemap (I think the design accounted for this somehow, but this is way more important than I realized)

See the spacing in the PR image. Note that I'm waiting on @paulirish to publish a new version that supports the spacing option. Until then, yea it is all bunched up and awful.

"All" button in top right has cursor pointer but doesn't seem to do anything yet or have a purpose without the other views, puntable?

Could, do you think the associated code is worth separating?

the hover border can get hidden sometimes, z-index issue or something maybe?

idk

patrickhulce · 2020-10-13T17:32:56Z

Could, do you think the associated code is worth separating?

No, not anymore once I started reviewing and came up with a much larger 3-way split :)

connorjclark requested a review from a team as a code owner October 9, 2020 23:47

connorjclark requested review from paulirish and removed request for a team October 9, 2020 23:47

connorjclark commented Oct 9, 2020

View reviewed changes

google-cla bot added the cla: yes label Oct 9, 2020

devtools-bot assigned paulirish Oct 9, 2020

devtools-bot added the waiting4reviewer label Oct 9, 2020

connorjclark commented Oct 9, 2020

View reviewed changes

vercel bot deployed to Preview October 10, 2020 00:01 View deployment

treemap: first pass on Lighthouse Treemap

86772a7

connorjclark force-pushed the minimal-treemap branch from 7c142a5 to 86772a7 Compare October 10, 2020 00:04

vercel bot requested a deployment to Preview October 10, 2020 00:04 Pending

vercel bot deployed to Preview October 10, 2020 00:04 View deployment

now build

b92f793

vercel bot deployed to Preview October 10, 2020 00:07 View deployment

css

5d2693d

vercel bot deployed to Preview October 10, 2020 00:12 View deployment

lateset

b91150f

vercel bot deployed to Preview October 12, 2020 19:50 View deployment

oops

5ab0d0d

vercel bot deployed to Preview October 12, 2020 20:47 View deployment

patrickhulce self-assigned this Oct 13, 2020

patrickhulce reviewed Oct 13, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Oct 13, 2020

some tweaks

cafb2f3

vercel bot deployed to Preview October 14, 2020 18:06 View deployment

brendankenny mentioned this pull request Oct 14, 2020

misc(build): refactor viewer bundler into reusable GhPagesApp #11564

Merged

update

df39217

vercel bot had a problem deploying to Preview November 5, 2020 20:04 Failure

use GhPagesApp

6682ffb

vercel bot had a problem deploying to Preview November 5, 2020 20:10 Failure

connorjclark mentioned this pull request Nov 5, 2020

misc(treemap): initialize app structure #11635

Merged

connorjclark mentioned this pull request Nov 13, 2020

report: treemap opener, ?dev for localhost viewer and treemap #11667

Merged

connorjclark closed this Nov 17, 2020

brendankenny deleted the minimal-treemap branch May 18, 2021 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

treemap: first pass on Lighthouse Treemap #11545

treemap: first pass on Lighthouse Treemap #11545

connorjclark commented Oct 9, 2020 •

edited

Loading

connorjclark Oct 9, 2020

patrickhulce Oct 13, 2020

connorjclark Oct 13, 2020

connorjclark Oct 9, 2020

patrickhulce Oct 13, 2020

connorjclark Oct 9, 2020

connorjclark Oct 9, 2020

patrickhulce commented Oct 13, 2020

patrickhulce left a comment •

edited

Loading

patrickhulce Oct 13, 2020

patrickhulce Oct 13, 2020

patrickhulce Oct 13, 2020

patrickhulce Oct 13, 2020

patrickhulce Oct 13, 2020

patrickhulce Oct 13, 2020

connorjclark Oct 13, 2020

patrickhulce Oct 13, 2020

patrickhulce Oct 13, 2020

patrickhulce Oct 13, 2020

connorjclark Oct 13, 2020

patrickhulce Oct 13, 2020

patrickhulce Oct 13, 2020

connorjclark Oct 13, 2020

connorjclark commented Oct 13, 2020

patrickhulce commented Oct 13, 2020

		@@ -27,6 +27,9 @@

		/** @typedef {import('./dom')} DOM */

		const VIEWER_ORIGIN = 'http://localhost:8000';

	* @param {Object} data
	* @template {{lhresult: LH.Result}} T
	* @param {T} data

	# visit http://localhost:8000/treemap/?debug
	open http://localhost:8000/treemap/?debug

treemap: first pass on Lighthouse Treemap #11545

treemap: first pass on Lighthouse Treemap #11545

Conversation

connorjclark commented Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Oct 13, 2020

patrickhulce left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Oct 13, 2020

patrickhulce commented Oct 13, 2020

connorjclark commented Oct 9, 2020 •

edited

Loading

patrickhulce left a comment •

edited

Loading