-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report(links): include utm params in links to docs #7441
Conversation
@@ -82,7 +82,9 @@ class CategoryRenderer { | |||
const titleEl = this.dom.find('.lh-audit__title', auditEl); | |||
titleEl.appendChild(this.dom.convertMarkdownCodeSnippets(audit.result.title)); | |||
this.dom.find('.lh-audit__description', auditEl) | |||
.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description)); | |||
.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description, { | |||
rating: Util.calculateRating(audit.result.score, scoreDisplayMode), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rviscomi Right now this means notApplicable will show up as "pass", do we need to separate them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be best to annotate it accordingly but not important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than fallback plan!
@@ -27,6 +27,8 @@ class DOM { | |||
constructor(document) { | |||
/** @type {Document} */ | |||
this._document = document; | |||
/** @type {string} */ | |||
this._lighthouseChannel = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a fallback or default for reports that won't have the channel yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a fallback or default for reports that won't have the channel yet?
yeah, it would be nice to do a pretty obvious fallback (e.g. 'unknown'
or something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
@@ -27,6 +27,8 @@ class DOM { | |||
constructor(document) { | |||
/** @type {Document} */ | |||
this._document = document; | |||
/** @type {string} */ | |||
this._lighthouseChannel = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a fallback or default for reports that won't have the channel yet?
yeah, it would be nice to do a pretty obvious fallback (e.g. 'unknown'
or something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM2
Summary
Include utm parameters in the audit link URLs to better understand how people arrive at the docs.
I think this is fine with DevTools and Chrome isn't overriding anything relevant: https://cs.chromium.org/chromium/src/third_party/blink/renderer/devtools/front_end/audits2/Audits2Panel.js?l=148&rcl=24c2704773b6d26f3fa65985dc54fb49eb449a6b
Related Issues/PRs
Another go at this PR
Closes #4663