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.

Sign up for GitHub

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

Jump to bottom

report: eliminate cards and list details #4789

Merged
merged 6 commits into from
Mar 21, 2018
Merged

report: eliminate cards and list details #4789

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
971b855
report: eliminate cards and list details
patrickhulce Mar 16, 2018
c9cc2a5
remove extra dom size tests
patrickhulce Mar 16, 2018
aa28278
feedback
patrickhulce Mar 17, 2018
e113932
Merge branch 'master' into kill_list_cards
patrickhulce Mar 21, 2018
0941df8
rebase fixtures
patrickhulce Mar 21, 2018
5ad0284
delete shell scriptg
patrickhulce Mar 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 0 additions & 103 deletions lighthouse-cli/test/fixtures/dobetterweb/domtester.html
View file
Open in desktop

This file was deleted.

88 changes: 0 additions & 88 deletions lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,94 +186,6 @@ module.exports = [
},
},
},
}, {
initialUrl: 'http://localhost:10200/dobetterweb/domtester.html?smallDOM',
url: 'http://localhost:10200/dobetterweb/domtester.html?smallDOM',
audits: {
'dom-size': {
score: 1,
extendedInfo: {
value: {
0: {value: '1,324'},
1: {value: '7'},
2: {value: '1,303'},
},
},
details: {
items: {
0: {value: '1,324'},
1: {value: '7'},
2: {value: '1,303'},
},
},
},
},
}, {
initialUrl: 'http://localhost:10200/dobetterweb/domtester.html?largeDOM&withShadowDOM',
url: 'http://localhost:10200/dobetterweb/domtester.html?largeDOM&withShadowDOM',
audits: {
'dom-size': {
score: 0,
extendedInfo: {
value: {
0: {value: '6,037'},
1: {value: '9'},
2: {value: '6,003'},
},
},
details: {
items: {
0: {value: '6,037'},
1: {value: '9'},
2: {value: '6,003'},
},
},
},
},
}, {
initialUrl: 'http://localhost:10200/dobetterweb/domtester.html?withShadowDOM',
url: 'http://localhost:10200/dobetterweb/domtester.html?withShadowDOM',
audits: {
'dom-size': {
score: 1,
extendedInfo: {
value: {
0: {value: '37'},
1: {value: '9'},
2: {value: '9'},
},
},
details: {
items: {
0: {value: '37'},
1: {value: '9'},
2: {value: '9'},
},
},
},
},
}, {
initialUrl: 'http://localhost:10200/dobetterweb/domtester.html?ShadowRootWithManyChildren',
url: 'http://localhost:10200/dobetterweb/domtester.html?ShadowRootWithManyChildren',
audits: {
'dom-size': {
score: 1,
extendedInfo: {
value: {
0: {value: '33'},
1: {value: '7'},
2: {value: '9'},
},
},
details: {
items: {
0: {value: '33'},
1: {value: '7'},
2: {value: '9'},
},
},
},
},
}, {
initialUrl: 'http://localhost:10200/online-only.html',
url: 'http://localhost:10200/online-only.html',
Expand Down
27 changes: 13 additions & 14 deletions lighthouse-core/audits/accessibility/axe-audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,29 +34,28 @@ class AxeAudit extends Audit {
const violations = artifacts.Accessibility.violations || [];
const rule = violations.find(result => result.id === this.meta.name);

let nodeDetails = [];
let items = [];
if (rule && rule.nodes) {
nodeDetails = rule.nodes.map(node => ({
type: 'node',
selector: Array.isArray(node.target) ? node.target.join(' ') : '',
path: node.path,
snippet: node.snippet,
items = rule.nodes.map(node => ({
node: {
type: 'node',
selector: Array.isArray(node.target) ? node.target.join(' ') : '',
path: node.path,
snippet: node.snippet,
},
}));
}

const headings = [
{key: 'node', itemType: 'node', text: 'Failing Elements'},
];

return {
rawValue: typeof rule === 'undefined',
extendedInfo: {
value: rule,
},
details: {
type: 'list',
header: {
type: 'text',
text: 'View failing elements',
},
items: nodeDetails,
},
details: Audit.makeTableDetails(headings, items),
};
}
}
Expand Down
51 changes: 18 additions & 33 deletions lighthouse-core/audits/dobetterweb/dom-size.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,24 +47,15 @@ class DOMSize extends Audit {
};
}


/**
* @param {!Artifacts} artifacts
* @return {!AuditResult}
*/
static audit(artifacts) {
const stats = artifacts.DOMStats;

/**
* html >
* body >
* div >
* span
*/
const depthSnippet = stats.depth.pathToElement.reduce((str, curr, i) => {
return `${str}\n` + ' '.repeat(i) + `${curr} >`;
}, '').replace(/>$/g, '').trim();
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

lighthouse/lighthouse-core/gather/gatherers/accessibility.js

Lines 76 to 85 in 7c7695b

/**
* 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 •
edited
Loading

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


// Use the CDF of a log-normal distribution for scoring.
// <= 1500: score≈1
Expand All @@ -76,34 +67,28 @@ class DOMSize extends Audit {
SCORING_MEDIAN
);

const cards = [{
title: 'Total DOM Nodes',
value: Util.formatNumber(stats.totalDOMNodes),
target: `< ${Util.formatNumber(MAX_DOM_NODES)} nodes`,
}, {
title: 'DOM Depth',
value: Util.formatNumber(stats.depth.max),
snippet: depthSnippet,
target: `< ${Util.formatNumber(MAX_DOM_TREE_DEPTH)}`,
}, {
title: 'Maximum Children',
value: Util.formatNumber(stats.width.max),
snippet: widthSnippet,
target: `< ${Util.formatNumber(MAX_DOM_TREE_WIDTH)} nodes`,
}];
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

{key: 'width', itemType: 'text', text: 'Maximum Children'},
];

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

@paulirish paulirish Mar 16, 2018 •
edited
Loading

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 •
edited
Loading

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 👍

width: `${Util.formatNumber(stats.width.max)} (${widthSnippet})`,
},
];

return {
score,
rawValue: stats.totalDOMNodes,
displayValue: `${Util.formatNumber(stats.totalDOMNodes)} nodes`,
extendedInfo: {
value: cards,
},
details: {
type: 'cards',
header: {type: 'text', text: 'View details'},
items: cards,
value: items,
},
details: Audit.makeTableDetails(headings, items),
};
}
}
Expand Down
20 changes: 9 additions & 11 deletions lighthouse-core/audits/dobetterweb/password-inputs-can-be-pasted-into.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,22 +29,20 @@ class PasswordInputsCanBePastedIntoAudit extends Audit {
static audit(artifacts) {
const passwordInputsWithPreventedPaste = artifacts.PasswordInputsWithPreventedPaste;

const items = passwordInputsWithPreventedPaste.map(input => ({
node: {type: 'node', snippet: input.snippet},
}));

const headings = [
{key: 'node', itemType: 'node', text: 'Failing Elements'},
];

return {
rawValue: passwordInputsWithPreventedPaste.length === 0,
extendedInfo: {
value: passwordInputsWithPreventedPaste,
},
details: {
type: 'list',
header: {
type: 'text',
value: 'Password inputs that prevent pasting into',
},
items: passwordInputsWithPreventedPaste.map(input => ({
type: 'text',
value: input.snippet,
})),
},
details: Audit.makeTableDetails(headings, items),
};
}
}
Expand Down
14 changes: 9 additions & 5 deletions lighthouse-core/audits/is-on-https.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,17 +58,21 @@ class HTTPS extends Audit {
displayValue = `${insecureRecords.length} insecure request found`;
}

const items = insecureRecords.map(record => ({
url: record.url,
}));

const headings = [
{key: 'url', itemType: 'url', text: 'Insecure URL'},
];

return {
rawValue: insecureRecords.length === 0,
displayValue,
extendedInfo: {
value: insecureRecords,
},
details: {
type: 'list',
header: {type: 'text', text: 'Insecure URLs:'},
items: insecureRecords.map(record => ({type: 'url', value: record.url})),
},
details: Audit.makeTableDetails(headings, items),
};
});
}
Expand Down
Loading