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(lightwallet): add resource-summary audit #8522

Merged
merged 7 commits into from
May 1, 2019
Merged

core(lightwallet): add resource-summary audit #8522

merged 7 commits into from
May 1, 2019

Conversation

khempenius
Copy link
Collaborator

@khempenius khempenius commented Apr 22, 2019

Summary
This implements the diagnostic portion of LightWallet.

The budgets table portion of LightWallet will be implemented as a separate audit that has a dependency on this audit.

image

Related Issues/PRs

#8675, #8427, #8709, #8708,#8522, #8539

Screen Shot 2019-04-29 at 8 55 04 AM

@khempenius khempenius changed the title LightWallet: add resource-summary audit feat(lightWallet): add resource-summary audit Apr 22, 2019
@khempenius khempenius changed the title feat(lightWallet): add resource-summary audit feat(lightwallet): add resource-summary audit Apr 22, 2019
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

Awesome stuff @khempenius! 🎉

You mention we're going to add a separate audit for the budgets themselves. Is the long-term plan to replace the total-byte-weight audit with this one then?

lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-test.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Show resolved Hide resolved
lighthouse-core/test/results/sample_v2.json Outdated Show resolved Hide resolved
title: 'Keep request counts and file sizes small',
/** Description of a Lighthouse audit that tells the user that they can setup a budgets for the quantity and size of page resources. No character length limits. */
description: 'To set budgets for the quantity and size of page resources,' +
' add a budgets.json file.',
Copy link
Member

Choose a reason for hiding this comment

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

I know we don't have one, but we should get a [Learn more] link in here to link up to some docs for how they adopt the json. I think users would have a hard time figuring out what to do right now.

We could start with a .md in the repo and later do some nice docs.
not blocking this PR on user docs, but let's definitely put it on the queue.

Copy link
Member

Choose a reason for hiding this comment

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

not blocking this PR on user docs, but let's definitely put it on the queue.

how do we not have a lightwallet tracking issue? :)

added to #8331 in the meantime

lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
@paulirish paulirish mentioned this pull request Apr 29, 2019
67 tasks
lighthouse-core/test/results/sample_v2.json Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
title: 'Keep request counts and file sizes small',
/** Description of a Lighthouse audit that tells the user that they can setup a budgets for the quantity and size of page resources. No character length limits. */
description: 'To set budgets for the quantity and size of page resources,' +
' add a budgets.json file.',
Copy link
Member

Choose a reason for hiding this comment

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

not blocking this PR on user docs, but let's definitely put it on the queue.

how do we not have a lightwallet tracking issue? :)

added to #8331 in the meantime

lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-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.

will need to re-run yarn update:sample-json

types/budget.d.ts Outdated Show resolved Hide resolved
lighthouse-core/report/html/report-styles.css Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-test.js Outdated Show resolved Hide resolved
@brendankenny
Copy link
Member

the i18n description suggestions are just very quickly written suggestions, feel free to change more

@khempenius
Copy link
Collaborator Author

All changes made except updating json samples. Those keep changing, so I will do that once everything else looks good.

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

All changes made except updating json samples. Those keep changing

yeah, it can be annoying. A number of tests use sample_v2.json, so it's hard to judge correctness until that's been updated first

lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/lib/i18n/i18n.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

lookin' real 😎

lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
/** Imperative title of a Lighthouse audit that tells the user to minimize the size and quantity of resources used to load the page. */
title: 'Keep request counts and transfer sizes small',
/** Description of a Lighthouse audit that tells the user that they can setup a budgets for the quantity and size of page resources. No character length limits. */
description: 'To set budgets for the quantity and size of page resources,' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry if I already said this but are we gonna link this up to a doc? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was planning on doing that in a separate PR once we know where that lives.

Copy link
Member

Choose a reason for hiding this comment

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

I also added it to #8331 in the lightwallet section

lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/resource-summary-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 description update?)

lighthouse-core/audits/resource-summary.js Outdated Show resolved Hide resolved
/** Imperative title of a Lighthouse audit that tells the user to minimize the size and quantity of resources used to load the page. */
title: 'Keep request counts and transfer sizes small',
/** Description of a Lighthouse audit that tells the user that they can setup a budgets for the quantity and size of page resources. No character length limits. */
description: 'To set budgets for the quantity and size of page resources,' +
Copy link
Member

Choose a reason for hiding this comment

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

I also added it to #8331 in the lightwallet section

lighthouse-core/audits/resource-summary.js Show resolved Hide resolved
@khempenius
Copy link
Collaborator Author

Description updated 👍

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM!

.sort((a, b) => {
return b.size - a.size;
});
const tableItems = otherRows.concat(thirdPartyRow);
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

@brendankenny
Copy link
Member

🚢 🚢

@brendankenny brendankenny merged commit 9323a55 into GoogleChrome:master May 1, 2019
@brendankenny brendankenny changed the title feat(lightwallet): add resource-summary audit core(lightwallet): add resource-summary audit May 4, 2019
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.

5 participants