-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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): initialize app structure #11635
Conversation
Should the commits for this be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the split up, much easier to follow! 🎉 💯
m = s.getElementsByTagName(o)[0]; a.async = 1; a.src = g; m.parentNode.insertBefore(a, m) | ||
})(window, document, 'script', 'https://www.google-analytics.com/analytics.js', 'ga'); | ||
|
||
ga('create', 'UA-85519014-2', 'auto'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the same analytics account as the viewer? I guess that's what we want when served over same domain anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya
lighthouse-treemap/app/src/main.js
Outdated
*/ | ||
function injectOptions() { | ||
// @ts-expect-error | ||
if (!window.__injected) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this mean...?
if (!window.__injected) return; | |
// Don't inject again if we've already injected the options. | |
if (window.__injected) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if so my dream about syncing the UI state to this seems to be DOA :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I refactored this a bit, I think I was mistaken that a separated boolean was needed here)
What do you mean by syncing UI state? For the Ctrl-S document save? I suppose we could delete and keep replacing this script tag as the user changes the view, to facilitate that. Although I doubt many will know to do this, and a better future plan would be a gist-hosted solution.
type Node = _Node; | ||
} | ||
|
||
interface WebTreeMapOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these aren't used yet right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope. Just types for the webtreemap library. https://github.com/paulirish/webtreemap-cdt#options
lighthouse-treemap/app/src/main.js
Outdated
|
||
async function main() { | ||
if (window.__TREEMAP_OPTIONS) { | ||
init(window.__TREEMAP_OPTIONS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init(window.__TREEMAP_OPTIONS); | |
// Prefer the hardcoded options from a saved HTML file above all. | |
init(window.__TREEMAP_OPTIONS); |
is this the only situation that this applies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup!
|
||
declare global { | ||
module Treemap { | ||
interface Options { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this eventually have options in it too?
right now options feels like a bit of a misnomer, payload
, initialization data
, do any of those seem right?
I'm mostly pushing on this a bit because it feels like we are about to get some UI state for this app, which feels distinctly different from an initial payload. Maybe it's the intention to have all UI state configurable from the initial payload though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's the intention to have all UI state configurable from the initial payload though?
Yup. The UI state is called "Mode" in the WIP branch: https://github.com/GoogleChrome/lighthouse/compare/viewer-treemap-2#diff-c3c7c9ceaada02e42e7ba0b22d920ac101a4747c1742eae23000f6c7627c6f3bR13 The idea is that some bits of the LH report will set a mode
to show a certain view.
How active do you expect treemap viewer to be once the bulk is built? If the answer is "as active as viewer" I'd vote |
|
|
||
<script src="src/bundled.js"></script> | ||
<script> | ||
(function (i, s, o, g, r, a, m) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment here that summarizes what's happening could be helpful if it's not too much trouble. Personally, at first glance this bit threw me for a loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meant-to-be-unreadable code for google analytics :) it's what they give to you for copy/paste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@patrickhulce what further changes are you requesting? |
@@ -0,0 +1,28 @@ | |||
import '../../types/treemap'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any errors in vscode now, but yarn tsc -p lighthouse-treemap/
fails spectacularly. @brendankenny I'm quite lost here, do you have any advice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think typeRoots
might be your problem
I don't know yet I thought you were still working on this, sorry. Is this awaiting my feedback? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty much LGTM, just the types issue and cleanup of outstanding GA question
* @param {LH.Treemap.Options} options | ||
*/ | ||
function injectOptions(options) { | ||
if (window.__treemapOptions) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely fits now, so not worth discussing much, but can I reserve the right to object to options
if this turns into state
in future PRs? 😃
lighthouse-treemap/tsconfig.json
Outdated
{ | ||
"extends": "../tsconfig", | ||
"compilerOptions": { | ||
"typeRoots": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this actually includes the types like you want it to, does it? That might be your problem, most of our types aren't packages, they're just files that should be in include
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thanks!
@@ -0,0 +1,28 @@ | |||
import '../../types/treemap'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think typeRoots
might be your problem
ref #11254, split off from #11545
Basic app structure, including build and puppeteer tests. Simply prints the LHR to the page.