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(audits): point PWA audit description links to web.dev #9539

Merged
merged 5 commits into from
Aug 24, 2019
Merged

core(audits): point PWA audit description links to web.dev #9539

merged 5 commits into from
Aug 24, 2019

Conversation

mfriesenhahn
Copy link
Collaborator

@mfriesenhahn mfriesenhahn commented Aug 9, 2019

Summary
Points "Learn more" links in PWA audits to relevant web.dev guides.

Note
web.dev currently doesn't have a post for the Provides a valid apple-touch-icon audit. We'll try to draft one quickly.

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.

LGTM other than the redirects one

Do we want to add one for the PWA overall "Learn more" link?

image

(Note for other reviewers that the HTTPS one is covered by the best practices PR #9538 )

@@ -17,7 +17,7 @@ class RedirectsHTTP extends Audit {
title: 'Redirects HTTP traffic to HTTPS',
failureTitle: 'Does not redirect HTTP traffic to HTTPS',
description: 'If you\'ve already set up HTTPS, make sure that you redirect all HTTP ' +
'traffic to HTTPS. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/http-redirects-to-https).',
'traffic to HTTPS. [Learn more](https://web.dev/redirects-http).',
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doc seems to give the opposite advice, we want them to redirect their http traffic to https and the doc is mostly talking about avoiding redirects

@patrickhulce
Copy link
Collaborator

Oh and I don't know if we plan to move the PWA checklist over too or not

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM

@exterkamp
Copy link
Member

I'm wondering @paulirish if swap-locale-test should roundtrip from en-XL instead of es? Will it fail like this if we change links/change strings every time?

@brendankenny
Copy link
Member

I'm wondering @paulirish if swap-locale-test should roundtrip from en-XL instead of es? Will it fail like this if we change links/change strings every time?

shouldn't it be ok and this just needs an update:sample-json? Since the swap is by message id, the en and es will just have different links, but they'll still trade back and forth

@brendankenny
Copy link
Member

brendankenny commented Aug 22, 2019

yeah, we just checked and @mfriesenhahn you'll just need to run yarn update:sample-json to get the strings updated across the needed files

jk I did it :)

@googlebot
Copy link

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.

@GoogleChrome GoogleChrome deleted a comment from googlebot Aug 22, 2019
@brendankenny brendankenny changed the title core(audits): Point PWA audit links to web.dev core(audits): point PWA audit description links to web.dev Aug 24, 2019
@brendankenny brendankenny merged commit a1e05d6 into GoogleChrome:master Aug 24, 2019
@mfriesenhahn
Copy link
Collaborator Author

Do we want to add one for the PWA overall "Learn more" link?

I discussed with @exterkamp, and we were thinking it'd make sense to leave these as-is for now until we get the relevant info migrated to web.dev. Afterward, we can point those links to the landing page for each collection, if that SGTY.

@mfriesenhahn
Copy link
Collaborator Author

@patrickhulce, I've copied your content comments to web.dev issue 1343 for tracking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants