-
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 all 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,33 @@ document.addEventListener('DOMContentLoaded', _ => { | |
const optionsList = document.body.querySelector('.options__list'); | ||
const okButton = document.getElementById('ok'); | ||
|
||
function getLighthouseVersion() { | ||
return chrome.runtime.getManifest().version; | ||
} | ||
|
||
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 += '**Error Message**: ' + err.message + '\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. The URL the user was running LH against would also be helpful to add to the bug report. 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 thought about that, but is thought it could be considered invasive. Thoughts? 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. Perhaps we could have a placeholder for them to fill in?
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, it could be invasive. Most of the extension issues we've seen with an actual caught error (as opposed to something more subtle, like -1 in FMP) have been tracked down without the URL. We can add the optional prompt for it later if we find ourselves asking for it more often than not |
||
|
||
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 +159,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.
nice find!