-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report: correct audit titles that should not be capitalized #9419
Conversation
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.
whoops sorry I hate it when I forget to flush review comments :(
@@ -15,7 +15,7 @@ const MainThreadTasks = require('../computed/main-thread-tasks.js'); | |||
|
|||
const UIStrings = { | |||
/** Title of a Lighthouse audit that identifies the code on the page that the user doesn't control. This is shown in a list of audits that Lighthouse generates. */ | |||
title: 'Third-Party Usage', | |||
title: 'Third-Party usage', |
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 just convert this to a command like Limit third-party usage
?
I think that's the biggest way it's an odd man out in diagnostics. right now it's like the only title-style audit name so the capitalization makes sense there.
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.
+1 to adding a verb. maybe Minimize third-party usage
?
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@meggynw can you review these changes? wanted to make sure they are reasonable to you. |
@@ -15,7 +15,7 @@ const MainThreadTasks = require('../computed/main-thread-tasks.js'); | |||
|
|||
const UIStrings = { | |||
/** Title of a Lighthouse audit that identifies the code on the page that the user doesn't control. This is shown in a list of audits that Lighthouse generates. */ | |||
title: 'Third-Party Usage', | |||
title: 'Third-Party usage', |
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.
+1 to adding a verb. maybe Minimize third-party usage
?
@@ -11,7 +11,7 @@ const ComputedChains = require('../computed/critical-request-chains.js'); | |||
|
|||
const UIStrings = { | |||
/** Imperative title of a Lighthouse audit that tells the user to reduce the depth of critical network requests to enhance initial load of a page. Critical request chains are series of dependent network requests that are important for page rendering. For example, here's a 4-request-deep chain: The biglogo.jpg image is required, but is requested via the styles.css style code, which is requested by the initialize.js javascript, which is requested by the page's HTML. This is displayed in a list of audit titles that Lighthouse generates. */ | |||
title: 'Minimize Critical Requests Depth', | |||
title: 'Avoid chaining critical requests', |
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.
yah this is an improvement in clarity
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.
LGTM!
looks like got some conflicts now though, sorry!
lighthouse-core/lib/i18n/en-US.json
Outdated
@@ -0,0 +1,1858 @@ | |||
{ | |||
"lighthouse-core/audits/accessibility/accesskeys.js | description": { |
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.
this seems like a mistaken file to commit or bad merge or something @connorjclark ?
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.
this seems like a mistaken file to commit or bad merge or something @connorjclark ?
fixed
87fa4c0
to
2990196
Compare
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.
LGTM
@@ -15,7 +15,7 @@ const MainThreadTasks = require('../computed/main-thread-tasks.js'); | |||
|
|||
const UIStrings = { | |||
/** Title of a diagnostic audit that provides details about the code on a web page that the user doesn't control (referred to as "third-party code"). This descriptive title is shown to users when the amount is acceptable and no user action is required. */ | |||
title: 'Third-Party usage', | |||
title: 'Limit third-party usage', |
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.
only remaining thing is @paulirish suggested Minimize third-party usage
as an alternative verb here
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
thx for flagging. addressed by brendan
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
Couldn't unsee once I noticed, so here's some trivial copy changes.
Before:
![image](https://user-images.githubusercontent.com/4071474/61571548-0c777280-aa49-11e9-8ffe-16f7583453c3.png)
After:
![image](https://user-images.githubusercontent.com/4071474/61571576-2e70f500-aa49-11e9-8825-b47e96a01317.png)