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: pwa #9105

Merged
merged 28 commits into from
Aug 9, 2019
Merged

i18n: pwa #9105

merged 28 commits into from
Aug 9, 2019

Conversation

exterkamp
Copy link
Member

@exterkamp exterkamp commented Jun 2, 2019

Summary
i18n PWA! 🇺🇸 🇵🇷 🇦🇸 🇪🇺 🇨🇳 🇮🇳 🇩🇪 🇧🇷

TODO

Follow up:

  • Move all PWA audits into a pwa/ folder

Part of: #7238
fixes: #9530

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.

exterkamp (n.) an i18n phenom

lighthouse-core/audits/content-width.js Show resolved Hide resolved
lighthouse-core/audits/content-width.js Outdated Show resolved Hide resolved
'in on the communications between your app and your users, and is a prerequisite for ' +
'HTTP/2 and many new web platform APIs. ' +
'[Learn more](https://developers.google.com/web/tools/lighthouse/audits/https).',
displayValue: `{itemCount, plural,
Copy link
Collaborator

Choose a reason for hiding this comment

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

just for my personal clarity, this is one that would be preserved in the new proposal as "real" ICU syntax for translators?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is sent to the translators as "real" ICU. Plurals, and ordinals are the only* ICU syntax passed on.

*afaik

lighthouse-core/audits/is-on-https.js Outdated Show resolved Hide resolved
@@ -53,6 +53,7 @@ class MultiCheckAudit extends Audit {
if (result.failures.length > 0) {
return {
score: 0,
// TODO(exterkamp): make this i18n-able.
Copy link
Collaborator

Choose a reason for hiding this comment

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

haven't we sorta cheated similar to this in the past like in error messages? seems like we could do bullets or something

Copy link
Member Author

Choose a reason for hiding this comment

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

Afaict we don't ever really use "failures" because it is just an auto combination of a list of strings into an explanation. I think we just use explanations manually in my experience. We might want to just migrate away from failures totally since this is all they do.

I added this to keep the PR scoped, but I think there is opportunity here for better i18n.

Copy link
Collaborator

Choose a reason for hiding this comment

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

are we tracking this in a mega i18n issue somewhere? if we intend to collect TODOs in the code, I wonder if it might be better to make them all TODO(i18n) or something easily findable when we need to make our next i18n impacting decisions

Copy link
Collaborator

Choose a reason for hiding this comment

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

ping :)

Copy link
Member

Choose a reason for hiding this comment

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

+1 we need a way to track this. I was also told that #7238 is super important for tracking this stuff, so let's actually track some of this stuff in there :P

lighthouse-core/audits/works-offline.js Outdated Show resolved Hide resolved
@@ -53,18 +64,16 @@ class HTTPS extends Audit {
.filter(record => !HTTPS.isSecureRecord(record))
.map(record => URL.elideDataURI(record.url));

const items = Array.from(new Set(insecureURLs)).map(url => ({url}));
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this to before displayValue calc so that the display value # matches the de-duplicated item count.

@exterkamp exterkamp added the i18n internationalization thangs label Jun 4, 2019
lighthouse-core/audits/installable-manifest.js Outdated Show resolved Hide resolved
lighthouse-core/audits/installable-manifest.js Outdated Show resolved Hide resolved
failureTitle: 'Does not redirect HTTP traffic to HTTPS',
/** Description of a Lighthouse audit that tells the user why they should direct HTTP traffic to HTTPS. 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: 'If you\'ve already set up HTTPS, make sure that you redirect all HTTP ' +
'traffic to HTTPS in order enable secure web features for all your users. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/http-redirects-to-https).',
Copy link
Member

Choose a reason for hiding this comment

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

was this edit from the original from an earlier discussion? It seems like a less universal thing to use as the incentive vs something like "If you\'ve already set up HTTPS, make sure that you redirect all HTTP traffic to HTTPS in order to keep acess to your site secure" or whatever.

(If you want to keep this version, it should at least be "in order to enable secure web features...")

Copy link
Collaborator

Choose a reason for hiding this comment

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

I seem to remember saying something like this. It seems to me that in the context of PWA the primary reason you care about this is for using a service worker for all your visitors. That's a very concrete and specific carrot compared to "make it secure so that it will be secure" but 🤷‍♂

Copy link
Member

Choose a reason for hiding this comment

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

I seem to remember saying something like this. It seems to me that in the context of PWA the primary reason you care about this is for using a service worker for all your visitors. That's a very concrete and specific carrot compared to "make it secure so that it will be secure" but 🤷‍♂

well the most important reason to make it secure is so it will be secure :D but very good point about the context and the whole point is to be checking something a dev might forgot to check that will undermine other things they're trying to do. So change SGTM, but still needs the typo fix

Suggested change
'traffic to HTTPS in order enable secure web features for all your users. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/http-redirects-to-https).',
'traffic to HTTPS in order to enable secure web features for all your users. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/http-redirects-to-https).',

lighthouse-core/test/audits/offline-start-url-test.js Outdated Show resolved Hide resolved
lighthouse-core/audits/manual/pwa-page-transitions.js Outdated Show resolved Hide resolved
lighthouse-core/audits/service-worker.js Outdated Show resolved Hide resolved
lighthouse-core/audits/service-worker.js Outdated Show resolved Hide resolved
lighthouse-core/audits/service-worker.js Outdated Show resolved Hide resolved
lighthouse-core/audits/service-worker.js Outdated Show resolved Hide resolved
lighthouse-core/audits/service-worker.js Outdated Show resolved Hide resolved
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
@@ -53,6 +53,7 @@ class MultiCheckAudit extends Audit {
if (result.failures.length > 0) {
return {
score: 0,
// TODO(#7238): make this i18n-able.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@brendankenny
Copy link
Member

I also noticed the apple-touch-icon PWA audit has some issues with its description (apple-touch-icon should really be backticked and maybe an object for what's being added to the homescreen?)

@exterkamp
Copy link
Member Author

I also noticed the apple-touch-icon PWA audit has some issues with its description (apple-touch-icon should really be backticked and maybe an object for what's being added to the homescreen?)

Done. Added fixes to PR description.

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.

we could live with the (current) apple-touch-icon.js indentation, but also need that "to" in redirects-http.js :)

otherwise LGTM! 🎉

lighthouse-core/audits/apple-touch-icon.js Outdated Show resolved Hide resolved
lighthouse-core/audits/redirects-http.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes i18n internationalization thangs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

apple-touch-icon description is malformed
6 participants