-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Move perfy stuff into Perf Diagnostics report aggregation. #1724
Conversation
@@ -29,7 +29,7 @@ class UnusedCSSRules extends Audit { | |||
return { | |||
category: 'CSS', | |||
name: 'unused-css-rules', | |||
description: 'Avoids loading unnecessary CSS', | |||
description: 'Unused CSS Rules', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lolz. These are getting changed every PR.
FWIW, I really like descriptions like "Unused CSS Rules", but when you match it with the green check, it looks like you're doing a good job. That was the feedback we got from several people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, yeah might be worth having a dedicated report messaging discussion just to iron out what we want to standardize on :)
@@ -33,7 +33,7 @@ class ScriptBlockingFirstPaint extends Audit { | |||
return { | |||
category: 'Performance', | |||
name: 'script-blocking-first-paint', | |||
description: 'Avoids `<script>` in head that delay first paint', | |||
description: 'Render-blocking `<script>`', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we be consistent? <script>
-> scripts?
@@ -40,7 +40,7 @@ class UsesOptimizedImages extends Audit { | |||
return { | |||
category: 'Images', | |||
name: 'uses-optimized-images', | |||
description: 'Avoids unoptimized images', | |||
description: 'Unoptimized Images', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lighthouse-core/config/default.json
Outdated
@@ -372,20 +359,19 @@ | |||
} | |||
}] | |||
}, { | |||
"name": "Performance Metrics", | |||
"name": "Performance diagnostics", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would "Performance" be enough?
@@ -29,7 +29,7 @@ class UnusedCSSRules extends Audit { | |||
return { | |||
category: 'CSS', | |||
name: 'unused-css-rules', | |||
description: 'Avoids loading unnecessary CSS', | |||
description: 'Unused CSS Rules', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, yeah might be worth having a dedicated report messaging discussion just to iron out what we want to standardize on :)
lighthouse-core/config/default.json
Outdated
"expectedValue": 0, | ||
"weight": 1 | ||
} | ||
"unused-css-rules": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perf.json already looks like this, correct?
791287f
to
5068e6d
Compare
PTAL. Updated. Also updated perf.json as @ebidel If you're down, I'd like to file a ticket to address our green-checks-next-to-"unused css rules" bug in the report. I've already added it to Lighthouse Report Requirements |
IMO we should keep "Avoids..." until we've figured out something different. That language was hashed out over a couple of PRs and had Kayce input. I'm on mobile, but there's a Pavel ticket already open that you can add to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the hot potato game of descriptions, but hopefully the result of Lighthouse Report Requirements will standardize this for us :)
Merged to fix #1775 and others. Will comment on #1613 and take the bug to address wording. Also #1750 by @sendilkumarn will help considerably with the icon/text combo. Also @ebidel sorry for merging despite you not being totally happy. |
Sorry for just catching up... Running a report on a site that does good things, I get: Which makes it look like LH is advocating oversized images, unused css, and having unoptimized images :\ I feel like these should be something like:
IOW, the description lines should always read as the thing we want the user to do. |
screenshot: