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.

Sign up for GitHub

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

Jump to bottom

report: expand on "learn more" links #14091

Merged
merged 25 commits into from
Jul 21, 2022
Merged

report: expand on "learn more" links #14091

merged 25 commits into from
Jul 21, 2022

Conversation

alexnj
Copy link
Member

@alexnj alexnj commented Jun 6, 2022

Fixes #12241.

I've taken a stab at expanding "Learn more" links in LH reports to be more descriptive and accessible. Most of the expansions fall into one of the two formats:

  1. "Learn more about foo" (majority)
  2. "Learn how to bar" (if the link target is a guide, or is about avoiding something, or if I thought the first one doesn't fit well).

Feel free to suggest better expansions and I'll make the edits.

@alexnj alexnj requested a review from a team as a code owner June 6, 2022 18:29
@alexnj alexnj requested review from connorjclark and removed request for a team June 6, 2022 18:29
@alexnj alexnj requested a review from brendankenny June 6, 2022 18:31
alexnj added a commit to alexnj/lighthouse that referenced this pull request Jun 13, 2022
@alexnj
report(dom): support code snippets within markdown links
817d5b2 
Provides code snippet support within report markdown links.
Related to GoogleChrome#14091.
@alexnj alexnj mentioned this pull request Jun 13, 2022
Merged
@alexnj alexnj changed the title wip report: expand "learn more" links report: expand "learn more" links Jun 23, 2022
adamraine
adamraine reviewed Jul 12, 2022
View reviewed changes
Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

Quick comment dump for Accessibility. These are really just nit suggestions for consistency and specificity. Feel free to ignore them if you think the current stuff works better.

lighthouse-core/audits/accessibility/aria-tooltip-name.js Show resolved Hide resolved
lighthouse-core/audits/accessibility/html-lang-valid.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/input-image-alt.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/link-name.js Show resolved Hide resolved
lighthouse-core/audits/accessibility/aria-hidden-body.js Outdated Show resolved Hide resolved
adamraine
adamraine reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

Comments for Byte Efficiency (mostly)

lighthouse-core/audits/accessibility/tabindex.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/td-headers-attr.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/meta-viewport.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/meta-refresh.js Outdated Show resolved Hide resolved
lighthouse-core/audits/byte-efficiency/efficient-animated-content.js Outdated Show resolved Hide resolved
lighthouse-core/audits/csp-xss.js Outdated Show resolved Hide resolved
paulirish
paulirish reviewed Jul 18, 2022
View reviewed changes
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.

as Adam did a11y and BE audits. I continued in the github order and completed through the dobetterweb audits, ending with uses-passive-event-listeners.js`

lighthouse-core/audits/dobetterweb/charset.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/doctype.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/js-libraries.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/notification-on-start.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/password-inputs-can-be-pasted-into.js Outdated Show resolved Hide resolved
lighthouse-core/audits/dobetterweb/uses-passive-event-listeners.js Outdated Show resolved Hide resolved
paulirish
paulirish approved these changes Jul 18, 2022
View reviewed changes
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.

aiight i went through the rest. I didnt doublecheck what adam did.

some small fixes here and there.

lgtm % the suggestions.

lighthouse-core/audits/work-during-interaction.js Outdated Show resolved Hide resolved
lighthouse-core/audits/viewport.js Outdated Show resolved Hide resolved
lighthouse-core/audits/unsized-images.js Outdated Show resolved Hide resolved
lighthouse-core/audits/server-response-time.js Outdated Show resolved Hide resolved
lighthouse-core/audits/seo/plugins.js Outdated Show resolved Hide resolved
lighthouse-core/audits/manual/pwa-each-page-has-url.js Outdated Show resolved Hide resolved
lighthouse-core/audits/manual/pwa-cross-browser.js Outdated Show resolved Hide resolved
lighthouse-core/audits/largest-contentful-paint-element.js Outdated Show resolved Hide resolved
lighthouse-core/audits/image-size-responsive.js Outdated Show resolved Hide resolved
lighthouse-core/audits/errors-in-console.js Outdated Show resolved Hide resolved
@alexnj @adamraine @paulirish
Apply suggestions from code review
e8f96f9 
Co-authored-by: Adam Raine <[email protected]>
Co-authored-by: Paul Irish <[email protected]>
adamraine
adamraine approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

LGTM

@connorjclark connorjclark changed the title report: expand "learn more" links report: expand on "learn more" links Jul 20, 2022
connorjclark
connorjclark reviewed Jul 20, 2022
View reviewed changes
lighthouse-core/audits/accessibility/aria-treeitem-name.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/bypass.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/duplicate-id-active.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/duplicate-id-aria.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/html-has-lang.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/image-alt.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/td-headers-attr.js Outdated Show resolved Hide resolved
lighthouse-core/audits/accessibility/valid-lang.js Outdated Show resolved Hide resolved
lighthouse-core/audits/apple-touch-icon.js Show resolved Hide resolved
lighthouse-core/audits/autocomplete.js Show resolved Hide resolved
@alexnj alexnj merged commit 5758465 into GoogleChrome:master Jul 21, 2022
@brendankenny brendankenny mentioned this pull request Oct 5, 2022
Merged
@alexnj alexnj mentioned this pull request Oct 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulirish paulirish paulirish approved these changes

@adamraine adamraine adamraine approved these changes

@connorjclark connorjclark connorjclark approved these changes

@brendankenny brendankenny Awaiting requested review from brendankenny

Assignees

@connorjclark connorjclark

Labels
waiting4reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lighthouse shouldn't use "Learn more" links in its report
5 participants
@alexnj @paulirish @connorjclark @adamraine @devtools-bot