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): render timing-budget audit #9925

Merged
merged 4 commits into from
Feb 7, 2020
Merged

core(lightwallet): render timing-budget audit #9925

merged 4 commits into from
Feb 7, 2020

Conversation

khempenius
Copy link
Collaborator

@khempenius khempenius commented Nov 6, 2019

Adds timing-budget to default config & renders table when applicable.

image

Related Issues/PRs
Issue: #8917

@natusvince
Copy link

Hi guys @paulirish @patrickhulce, and thanks to @khempenius for implementing budgets for timing metrics! Are there any plans for a review of this PR?

.lh-audit-group--budgets .lh-table tbody tr td:nth-child(5){
.lh-audit-group--budgets #performance-budget tbody tr td:nth-child(4),
.lh-audit-group--budgets #performance-budget tbody tr td:nth-child(5),
.lh-audit-group--budgets #timing-budget tbody tr td:nth-child(3){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.lh-audit-group--budgets #timing-budget tbody tr td:nth-child(3){
.lh-audit-group--budgets #timing-budget tbody tr td:nth-child(3) {

budgetsGroupEl.classList.add('lh-audit-group--budgets');
element.appendChild(budgetsGroupEl);
const budgetsGroupEl = this.renderAuditGroup(groups.budgets);
let showBudgetsGroup;
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be better to have a budgetTableEls array, push to it ~L176 (instead of append to budgetsGroupEl), and check it's non-empty before creating budgetsGroupEl.

@khempenius
Copy link
Collaborator Author

Bumping this up for review.

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

LGTM!

@connorjclark connorjclark merged commit 7ecfb08 into GoogleChrome:master Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants