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

i18n: localize audits in best-practices #9092

Merged
merged 14 commits into from
Jun 25, 2019
Merged

i18n: localize audits in best-practices #9092

merged 14 commits into from
Jun 25, 2019

Conversation

exterkamp
Copy link
Member

@exterkamp exterkamp commented May 31, 2019

Summary
i18n best practices!

Follow ups:

  • move errant best-prac audits into dobetterweb? move all best-practices audits to best-practices/ folder
  • ask @brendankenny about details and extended_info in best-practices

Part of: #7238

@exterkamp exterkamp added the i18n internationalization thangs label May 31, 2019
@patrickhulce
Copy link
Collaborator

move errant best-prac audits into dobetterweb?

By this do you mean move all best-practices audits into dobetterweb? If so, I'd instead propose moving all best-practices audits into a best-practices folder since dobetterweb doesn't have much meaning moving forward :D

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!! 🇪🇸 🇨🇳 🇩🇪

@exterkamp exterkamp marked this pull request as ready for review May 31, 2019 21:21
displayValue = `${Util.formatNumber(insecureURLs.length)} insecure requests found`;
} else if (insecureURLs.length === 1) {
displayValue = `${insecureURLs.length} insecure request found`;
if (items.length > 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this to be items.length instead of insecureURLs.length b/c items de-dupes the list and so the displayValue would show "3" when the test for dedupe wanted "2". Before it was a loose match but now it is exact, and made me hunt down why the displayValue differed from the extendedInfo.value and this is what I found.

Copy link
Member

Choose a reason for hiding this comment

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

I moved this to be items.length instead of insecureURLs.length b/c items de-dupes the list

yes, very good call

@exterkamp exterkamp mentioned this pull request Jun 3, 2019
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.

I reviewed from the top down through password-inputs....
Brendan did from the bottom up to uses-http2.

lighthouse-core/audits/deprecations.js Outdated Show resolved Hide resolved
lighthouse-core/audits/deprecations.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/doctype.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/doctype.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/doctype.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/no-document-write.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/no-document-write.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/no-document-write.js Outdated Show resolved Hide resolved
@@ -5060,6 +5060,23 @@
"warningHeader": "Warnings: "
},
"icuMessagePaths": {
Copy link
Member

Choose a reason for hiding this comment

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

icuMessagePaths

😱gets a quite a bit longer on each of these PRs :)

Copy link
Member Author

Choose a reason for hiding this comment

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

retweet 🐦

Copy link
Member Author

Choose a reason for hiding this comment

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

We could make ICUmsg paths optional, only add it when requested... 🤷‍♂

lighthouse-core/lib/i18n/i18n.js Outdated Show resolved Hide resolved
@@ -94,6 +94,8 @@ const UIStrings = {
seoCrawlingGroupTitle: 'Crawling and Indexing',
/* Description of the navigation section within the Search Engine Optimization (SEO) category. Within this section are audits with descriptive titles that highlight ways to make a website accessible to search engine crawlers. */
seoCrawlingGroupDescription: 'To appear in search results, crawlers need access to your app.',
/** Title of the Best Practices category of audits. This is displayed at the top of a list of audits focused on topics related to following web development best practices and accepted guidelines. Also used as a label of a score gauge; try to limit to 20 characters. */
Copy link
Member

Choose a reason for hiding this comment

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

I love how we write "Also used as a label of a score gauge" on the category titles as if anyone will know what that means :)

lighthouse-core/audits/is-on-https.js Outdated Show resolved Hide resolved
lighthouse-core/audits/is-on-https.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/uses-http2.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/uses-http2.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/uses-http2.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/uses-http2.js Outdated Show resolved Hide resolved
Co-Authored-By: Paul Irish <paul.irish@gmail.com>
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
@googlebot

This comment has been minimized.

@googlebot

This comment has been minimized.

@googlebot

This comment has been minimized.

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.

some more suggestions but otherwise LGTM

/** Description of a Lighthouse audit that tells the user why they should maintain the correct aspect ratios for all images. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
description: 'Image display dimensions should match natural aspect ratio. ' +
'[Learn more](https://developers.google.com/web/tools/lighthouse/audits/aspect-ratio).',
/** Warning that the sizing information cannot be collected for an image. */
Copy link
Member

Choose a reason for hiding this comment

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

happy if this uses a better description than my suggestion, but the current version doesn't match well with the message string :)

}`,
/** Table column header for the version of the Javascript library found. */
columnVersion: 'Library Version',
/** Table column header for the count of vulnerabilities found within a JavaSscript library. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** Table column header for the count of vulnerabilities found within a JavaSscript library. */
/** Label for a column in a data table; entries will be the counts of JavaScript-library vulnerabilities found. */

lighthouse-core/audits/dobetterweb/js-libraries.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/js-libraries.js Outdated Show resolved Hide resolved
@brendankenny brendankenny changed the title i18n: best-practices i18n: localize audits in best-practices Jun 25, 2019
@googlebot

This comment has been minimized.

@googlebot

This comment has been minimized.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: no i18n internationalization thangs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants