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(charset): add description link to web.dev guide #10689

Merged
merged 2 commits into from
May 5, 2020
Merged

Conversation

kaycebasques
Copy link
Contributor

@kaycebasques kaycebasques commented May 1, 2020

Summary

Links the charset audit description to the new web.dev guide dedicated to that topic.

Related Issues/PRs

GoogleChrome/web.dev#2771

@kaycebasques kaycebasques requested a review from a team as a code owner May 1, 2020 17:09
@kaycebasques kaycebasques requested review from patrickhulce and removed request for a team May 1, 2020 17:09
Copy link
Contributor Author

@kaycebasques kaycebasques left a comment

Choose a reason for hiding this comment

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

Please review the new guide as well.

@@ -24,7 +24,7 @@ const UIStrings = {
/** Description of a Lighthouse audit that tells the user why the charset needs to be defined early on. */
description: 'A character encoding declaration is required. It can be done with a <meta> tag ' +
'in the first 1024 bytes of the HTML or in the Content-Type HTTP response header. ' +
'[Learn more](https://www.w3.org/International/questions/qa-html-encoding-declarations).',
'[Learn more](https://web.dev/charset).',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brendankenny brendankenny changed the title Link to web.dev guide core(charset): add link to web.dev guide May 1, 2020
@brendankenny brendankenny changed the title core(charset): add link to web.dev guide core(charset): add description link to web.dev guide May 1, 2020
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, feel free to land once it goes live @kaycebasques

As for the docs themselves..

Might be nice to mention some of the performance consequences that prioritized the original issue too. That comment also calls out when it's an actual problem (it's based on the domain name) would be cool to have the docs say specifically when it happens too, but not a big deal. It looks good as-is too.

@kaycebasques
Copy link
Contributor Author

kaycebasques commented May 5, 2020

Updated the guide to mention the performance consideration and merged the documentation PR (page should be live in 10 minutes)

@kaycebasques
Copy link
Contributor Author

@patrickhulce I don't authorization to merge

image

@patrickhulce patrickhulce merged commit 1b93a1d into master May 5, 2020
@patrickhulce patrickhulce deleted the charset branch May 5, 2020 23:54
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.

4 participants