-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Extension: add report error button #944
Changes from 4 commits
5f5f9f7
0a3bdaf
286af63
33015d2
9029182
e78062d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,32 @@ document.addEventListener('DOMContentLoaded', _ => { | |
const optionsList = document.body.querySelector('.options__list'); | ||
const okButton = document.getElementById('ok'); | ||
|
||
function getLightHouseVersion() { | ||
return chrome.runtime.getManifest().version; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice find! |
||
} | ||
|
||
function getChromeVersion() { | ||
return /Chrome\/([0-9.]+)/.exec(navigator.userAgent)[1]; | ||
} | ||
|
||
function buildReportErrorLink(err) { | ||
const reportErrorEl = document.createElement('a'); | ||
reportErrorEl.className = 'button button--report-error'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have @robdodson in my head saying "just use a button" here. Should this be just a button? Alternatively we could make the link more link-like. I don't want to bikeshed here too much, we can always change it up later if we don't get it exactly right at first. @ebidel for a quick opinion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A button would be consistent with the existing HTML buttons, but would have a few small hoops to jump through, i.e. storing the link on a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in general buttons are for actions and links are for navigation. Not sure what the intent of this control is but happy to take a look in-person if that's any help There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, my point was more styling. It's a link acting like a link, but somewhat styled like a button :) However, it works and we've talked about reworking the popup as it keeps getting more crowded anyways, so we can address this at that point if need be. I'd like to get this landed now to help users out. |
||
|
||
let qsBody = '**Lighthouse Version**: ' + getLightHouseVersion() + '\n'; | ||
qsBody += '**Chrome Version**: ' + getChromeVersion() + '\n'; | ||
qsBody += '**Stack Trace**:\n' + err.stack; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: possible to surround the stack with `````s to turn it into a code block? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call and done. After placing it in a code block, I noticed the original error wasn't in the stack trace, so I also added that. |
||
|
||
const base = 'https://github.com/googlechrome/lighthouse/issues/new?'; | ||
const title = encodeURI('title=Lighthouse Extension Error'); | ||
const body = '&body=' + encodeURI(qsBody); | ||
|
||
reportErrorEl.href = base + title + body; | ||
reportErrorEl.textContent = 'Report Error'; | ||
reportErrorEl.target = '_blank'; | ||
return reportErrorEl; | ||
} | ||
|
||
let spinnerAnimation; | ||
|
||
function startSpinner() { | ||
|
@@ -132,16 +158,26 @@ document.addEventListener('DOMContentLoaded', _ => { | |
}) | ||
.catch(err => { | ||
let message = err.message; | ||
if (message.toLowerCase().startsWith('another debugger')) { | ||
|
||
const debuggerExists = message.toLowerCase().startsWith('another debugger'); | ||
const multipleTabs = message.toLowerCase().includes('multiple tabs'); | ||
|
||
if (debuggerExists) { | ||
message = 'You probably have DevTools open.' + | ||
' Close DevTools to use lighthouse'; | ||
} | ||
if (message.toLowerCase().includes('multiple tabs')) { | ||
if (multipleTabs) { | ||
message = 'You probably have multiple tabs open to the same origin.' + | ||
' Close the other tabs to use lighthouse.'; | ||
} | ||
|
||
feedbackEl.textContent = message; | ||
|
||
if (!multipleTabs && !debuggerExists) { | ||
feedbackEl.className = 'feedback-error'; | ||
feedbackEl.appendChild(buildReportErrorLink(err)); | ||
} | ||
|
||
stopSpinner(); | ||
background.console.error(err); | ||
}); | ||
|
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.
nit: we aren't always consistent with this, but lighthouse is one word, so should be
getLighthouseVersion()
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.
Done!