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

core(audit): make dom-size table prettier #6065

Merged
merged 7 commits into from
Sep 20, 2018
Merged

Conversation

exterkamp
Copy link
Member

Summary
Reformatted the dom-size output table. Converted the table to be more row-centric vs column centric. See issue #5220 for discussion and screenshots. Some CSS changes required from #6049 to format the code snippets and the left alignment of the text.

Related Issues/PRs
fixes: #5220
requires: #6049

columnDOMDepth: 'Maximum DOM Depth',
/** Label for the value of the maximum number of children any DOM node in the page has. */
columnDOMWidth: 'Maximum Children',
/** Table column header for the type of analytic found. */
Copy link
Member Author

Choose a reason for hiding this comment

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

@brendankenny @paulirish are these label comments sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

keep in mind that ~90 different linguists will be reading these descriptions to translate the text to another language. And they see them in isolation, without knowing necessarily what it looks like in the UI, or without having read any other descriptions of other strings. :)

in this case 'analytic' and 'snippet' aren't well understood terms, so that'd be hard for them.

Table column header for the type of statistic. These statistics describe how big the DOM is (count of DOM nodes, children, depth).

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.

nice nice

/** Label for the value of the maximum number of children any DOM node in the page has. */
columnDOMWidth: 'Maximum Children',
/** Table column header for the type of analytic found. */
columnCategory: 'Category',
Copy link
Member

Choose a reason for hiding this comment

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

bikeshed. wdyt about 'Statistic' or 'Metric'. cc @brendankenny @patrickhulce

Copy link
Member

Choose a reason for hiding this comment

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

I like either of those, maybe Metric more? Agreed they're slightly more descriptive than Category

Copy link
Member Author

Choose a reason for hiding this comment

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

I took the Category name from the Minimizes main thread work Audit table. So we can update the phrasing in every table if we like Metric more (which I agree is more descriptive than Category).

category_screenshot

Copy link
Member

Choose a reason for hiding this comment

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

heh, in that case I feel like "Category" is better (since it's actually bringing e.g. multiple Script Evaluation tasks together and grouping them into one line), but also see the appeal of consistency

columnDOMDepth: 'Maximum DOM Depth',
/** Label for the value of the maximum number of children any DOM node in the page has. */
columnDOMWidth: 'Maximum Children',
/** Table column header for the type of analytic found. */
Copy link
Member

Choose a reason for hiding this comment

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

keep in mind that ~90 different linguists will be reading these descriptions to translate the text to another language. And they see them in isolation, without knowing necessarily what it looks like in the UI, or without having read any other descriptions of other strings. :)

in this case 'analytic' and 'snippet' aren't well understood terms, so that'd be hard for them.

Table column header for the type of statistic. These statistics describe how big the DOM is (count of DOM nodes, children, depth).

lighthouse-core/audits/dobetterweb/dom-size.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/dom-size.js Outdated Show resolved Hide resolved
categoryDOMNodes: 'Total DOM Nodes',
/** Label for the numeric value of the maximum depth in the page's DOM tree. */
categoryDOMDepth: 'Maximum DOM Depth',
/** Label for the value of the maximum number of children any DOM node in the page has. */
Copy link
Member

Choose a reason for hiding this comment

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

Might add "This item described is the element that has the most children" just to be safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, might want to check the wording again.

lighthouse-core/lib/page-functions.js Outdated Show resolved Hide resolved
let prunedMatch = match[0];
ignore.forEach(attribute =>{
const ignoreRegex = new RegExp(attribute + '=".*?"');
prunedMatch = prunedMatch.split(ignoreRegex).join('');
Copy link
Member

Choose a reason for hiding this comment

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

wdyt about replacing with style="..." instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I refactored this function, kept the removal aspect, but it is much more clear. Thoughts? I could manually re-add the removed attr's after the Node is stringified, but that seems excessive.

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.

looking good; definitely like the attribute approach

/** Label for the value of the maximum number of children any DOM node in the page has. */
columnDOMWidth: 'Maximum Children',
/** Table column header for the type of analytic found. */
columnCategory: 'Category',
Copy link
Member

Choose a reason for hiding this comment

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

I like either of those, maybe Metric more? Agreed they're slightly more descriptive than Category

lighthouse-core/lib/page-functions.js Outdated Show resolved Hide resolved
lighthouse-core/lib/page-functions.js Show resolved Hide resolved
lighthouse-core/test/lib/page-functions-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/lib/page-functions-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/lib/page-functions-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/lib/page-functions-test.js Outdated Show resolved Hide resolved
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.

LGTM (with some last nits)

lighthouse-core/lib/page-functions.js Outdated Show resolved Hide resolved
lighthouse-core/test/lib/page-functions-test.js Outdated Show resolved Hide resolved
@brendankenny
Copy link
Member

you'll also need to run yarn update:sample-json to get the updated localization descriptions

lighthouse-core/audits/dobetterweb/dom-size.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/dom-size.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/dom-size.js Outdated Show resolved Hide resolved
@paulirish paulirish merged commit 2d00324 into master Sep 20, 2018
@paulirish
Copy link
Member

oochie coochie la la la. oochie coochie. 🎶

@paulirish paulirish deleted the refactor-dom-size branch September 20, 2018 22:16
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.

Audit: new dom nodes table is confusing
3 participants