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: adjust audit titles for consistency #5717

Merged
merged 3 commits into from
Jul 26, 2018
Merged

core: adjust audit titles for consistency #5717

merged 3 commits into from
Jul 26, 2018

Conversation

brendankenny
Copy link
Member

from lightsights discussions. Let the bikeshedding commence!

@@ -15,7 +15,7 @@ class CriticalRequestChains extends Audit {
static get meta() {
return {
id: 'critical-request-chains',
title: 'Critical Request Chains',
title: 'Minimize Critical Request Chains',
Copy link
Member Author

Choose a reason for hiding this comment

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

the audit is marked notApplicable if no requests are made, so everyone who gets the title could in theory minimize their chains. There might not be any benefit to doing that, but that's just a fundamental issue with the audit that needs to be addressed elsewhere :)

@brendankenny
Copy link
Member Author

@patrickhulce sorry, this will conflict with #5716, but at least it's small

@patrickhulce
Copy link
Collaborator

patrickhulce commented Jul 24, 2018

could this be done on top of #5716 :D (formerly #5692)

EDIT: oh you already commented about it 😞

@brendankenny
Copy link
Member Author

could this be done on top of #5716

that's fine. It should be easy

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.

optimizations only have failure titles (defined in .title). This leads to a big inconsistency when looking at all passed audits and their phrasing.

but we're not fixing that today.

Let's revisit phrasing later. PSI's approach (always failure title, customized description) is compelling.

@@ -244,7 +244,7 @@
},
"time-to-first-byte": {
"id": "time-to-first-byte",
"title": "Keep server response times low (TTFB)",
"title": "Server response times are short (TTFB)",
Copy link
Member

Choose a reason for hiding this comment

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

👀 TTFB is one for the non-translated strings / glossary.

@brendankenny
Copy link
Member Author

updated to live on top of the newly extracted UIStrings

@@ -204,7 +204,10 @@
"message": "Redirects introduce additional delays before the page can be loaded. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/redirects)."
},
"lighthouse-core/audits/time-to-first-byte.js | title": {
"message": "Keep server response times low (TTFB)"
"message": "Server response times are short (TTFB)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

low

:)

@@ -46,6 +46,6 @@ if [ $retVal -eq 0 ]; then
colorText "✅ PASS. No change in LHR." "$green"
else
colorText "❌ FAIL. LHR has changed." "$red"
echo "Run \`yarn update:sample-json\` to rebaseline the golden LHR."
echo "Run \`yarn i18n:collect-strings\` and \`yarn update:sample-json\` to rebaseline the golden LHR."
Copy link
Member Author

Choose a reason for hiding this comment

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

It's definitely confusing to update strings in audits but get no changes when you run yarn update:sample-json . This still isn't the best place for this advice (though it is needed) as there's no change to sample_v2.json until you extract the strings, so this test was actually passing (though yarn i18n:checks would fail on travis).

Should we add i18n:checks to diff:sample-json so the check does both? Or add i18n:collect-strings to update:sample-json so it's automatically done at the same time?

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

done

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.

this all works for me!

@brendankenny brendankenny merged commit 0421b6c into master Jul 26, 2018
@brendankenny brendankenny deleted the titletweaks branch July 26, 2018 20:36
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.

None yet

3 participants