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

report: eliminate cards and list details #4789

Merged
merged 6 commits into from
Mar 21, 2018
Merged

report: eliminate cards and list details #4789

merged 6 commits into from
Mar 21, 2018

Conversation

patrickhulce
Copy link
Collaborator

in the name of #4333

replaces them with tables for now
image
image
image

I think it's actually somewhat of an improvement for list ones :)

const items = [
{
totalNodes: Util.formatNumber(stats.totalDOMNodes),
depth: `${Util.formatNumber(stats.depth.max)} (${depthSnippet})`,
Copy link
Member

@paulirish paulirish Mar 16, 2018

Choose a reason for hiding this comment

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

i dont know if this display will scale.

e.g:

image

fwiw here is the same with the card:
image

url is https://tinyhousebuild.com/ btw

Copy link
Collaborator Author

@patrickhulce patrickhulce Mar 16, 2018

Choose a reason for hiding this comment

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

heh good catch, do we really need the snippet displayed if it was just a tooltip before?

just saw below I like outer HTML 👍

}];
const headings = [
{key: 'totalNodes', itemType: 'text', text: 'Total DOM Nodes'},
{key: 'depth', itemType: 'text', text: 'DOM Depth'},
Copy link
Member

Choose a reason for hiding this comment

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

let's add "Maximum" to this title

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

const widthSnippet = 'Element with most children:\n' +
stats.width.pathToElement[stats.width.pathToElement.length - 1];
const depthSnippet = stats.depth.pathToElement.join(' > ');
const widthSnippet = stats.width.pathToElement.join(' > ');
Copy link
Member

Choose a reason for hiding this comment

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

for the snippet i think we could simplify by providing the outerhtml snippet like

/**
* Gets the opening tag text of the given node.
* @param {!Node}
* @return {string}
*/
function getOuterHTMLSnippet(node) {
const reOpeningTag = /^.*?>/;
const match = node.outerHTML.match(reOpeningTag);
return match && match[0];
}

and forget this selector thing that will never scale.

i dont think this SOLVES the scaling problem, but it helps. maybe we put these snippets on a new row?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah agreed this is better 👍 I think we solve this with a new type in fancy report land with the other page stats (# network requests, etc)

Copy link
Collaborator Author

@patrickhulce patrickhulce Mar 17, 2018

Choose a reason for hiding this comment

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

I tried to get fancy and make it a proper node type but that looks a bit wonky as they get left-aligned while text right-aligned
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.

image

here's it as text which is what I'm going with for now, I expect this to change before 3.0 anyhow


node lighthouse-cli/test/fixtures/static-server.js &

node lighthouse-cli/index.js --output=json http://localhost:10200/dobetterweb/dbw_tester.html > ./lighthouse-core/test/results/sample_v2.json
Copy link
Member

Choose a reason for hiding this comment

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

i'd recommend using --output_path instead. it will help avoid any logging ending up there.

also with > the file is cleared the second this command starts, whereas with output_path it's just changed when everything is done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure will do

@@ -0,0 +1,13 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of checking in the latest-run artifacts folder for this fixture. so we'd have 1 script to run -A against it... and a second to refresh the artifacts. hows that sound?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so if I understand your proposal, is it two scripts

scripts/run-dash-a-against-dbw.sh
and
scripts/update-fixtures-with-latest-artifacts.sh

names pending? :) that sounds fine but not sure why we would necessarily need both. Is there a use case for running just the first?

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGreatTM on the details side :)

@brendankenny
Copy link
Member

is this waiting on #4793?

@paulirish
Copy link
Member

is this waiting on #4793?

yeah let's land that first. then we can nuke the .sh file from here and rebase. will be a superclean sample json diff :)

@patrickhulce
Copy link
Collaborator Author

much smaller diff now 👌 one more look?

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

still LGTM

@paulirish paulirish merged commit 483bd71 into master Mar 21, 2018
@paulirish paulirish deleted the kill_list_cards branch March 21, 2018 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants