Extension: add report error button #944

olingern · 2016-11-15T12:07:05Z

fixes #919

googlebot · 2016-11-15T12:12:23Z

CLAs look good, thanks!

brendankenny · 2016-11-15T16:56:49Z

Thanks for the PR!

I'll take a closer look at it shortly, but just as a quick drive by comment on the screenshot in #919, we'll definitely want the plugin and Chrome versions included in the bug report body.

Looks like you may have to parse the UA string to get the Chrome version (unless there's an extension API I'm missing), and you may need to pipe in the extension version through the background page.

brendankenny

In addition to the version numbers, we probably want the full error stack as well. Many of the error messages we give won't be enough to diagnose the problem the user is seeing

brendankenny · 2016-11-15T16:58:47Z

lighthouse-extension/app/src/popup.js

+
+      const base = 'https://github.com/googlechrome/lighthouse/issues/new?';
+      const title = 'title=Lighthouse%20Extension%20Error';
+      const body = '&body=Error:%20' + message;


the two cases handled by if statements above (devtools open and multiple tabs) should not trigger the report error button, since they're local things the user needs to fix

Good catch, thank ya!

ebidel · 2016-11-15T18:31:30Z

lighthouse-extension/app/src/popup.js

+      const body = '&body=Error:%20' + message;
+
+      reportErrorEl.href = base + title + body;
+      reportErrorEl.text = 'Report Error';


.text -> .textContent incase we use something other than HTMLAnchorElement in the future.

olingern · 2016-11-16T06:07:55Z

@ebidel @brendankenny
Thanks for the comments! I think everything should be there now. You can see how this looks below.

Plugin version ( found here )
Chrome version (thanks @brendankenny !)
Update link text property to be textContent

Updated view

brendankenny

looking good. Just some small style things left from me

brendankenny · 2016-11-16T08:41:31Z

lighthouse-extension/app/src/popup.js

@@ -37,6 +37,32 @@ document.addEventListener('DOMContentLoaded', _ => {
  const optionsList = document.body.querySelector('.options__list');
  const okButton = document.getElementById('ok');

+  function getLightHouseVersion() {


nit: we aren't always consistent with this, but lighthouse is one word, so should be getLighthouseVersion()

brendankenny · 2016-11-16T08:41:40Z

lighthouse-extension/app/src/popup.js

@@ -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;


brendankenny · 2016-11-16T08:43:38Z

lighthouse-extension/app/src/popup.js

+
+  function buildReportErrorLink(err) {
+    const reportErrorEl = document.createElement('a');
+    reportErrorEl.className = 'button button--report-error';


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.

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 data-issue-link attribute and binding an onclick function to the button. Nothing too crazy, but a link was easiest 😄

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

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.

brendankenny · 2016-11-16T08:46:20Z

lighthouse-extension/app/src/popup.js

+
+    let qsBody = '**Lighthouse Version**: ' + getLightHouseVersion() + '\n';
+    qsBody += '**Chrome Version**: ' + getChromeVersion() + '\n';
+    qsBody += '**Stack Trace**:\n' + err.stack;


nit: possible to surround the stack with `````s to turn it into a code block?

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.

ebidel · 2016-11-16T16:53:19Z

Think a link is fine because it's action is a URL taking you to a webpage.

On Wed, Nov 16, 2016, 1:17 AM Nick Olinger notifications@github.com wrote:

@olingern commented on this pull request.

In lighthouse-extension/app/src/popup.js
#944:

@@ -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;

}

function getChromeVersion() {

return /Chrome/([0-9.]+)/.exec(navigator.userAgent)[1];

}

function buildReportErrorLink(err) {

const reportErrorEl = document.createElement('a');

reportErrorEl.className = 'button button--report-error';

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
data-issue-link attribute and binding an onclick function to the button.
Nothing too crazy, but a link was easiest 😄

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#944, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAOigFSIJd1FPgoa84NO8xYZJIIymhTGks5q-sodgaJpZM4Kyc8C
.

ebidel · 2016-11-16T17:17:32Z

lighthouse-extension/app/src/popup.js

@@ -51,7 +51,8 @@ document.addEventListener('DOMContentLoaded', _ => {

    let qsBody = '**Lighthouse Version**: ' + getLightHouseVersion() + '\n';
    qsBody += '**Chrome Version**: ' + getChromeVersion() + '\n';
-    qsBody += '**Stack Trace**:\n' + err.stack;
+    qsBody += '**Error Message**: ' + err.message + '\n';
+    qsBody += '**Stack Trace**:\n ```' + err.stack + '```';


The URL the user was running LH against would also be helpful to add to the bug report.

I thought about that, but is thought it could be considered invasive. Thoughts?

Perhaps we could have a placeholder for them to fill in?

**URL:\n**: <Enter the URL that caused this error>

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

brendankenny

🐛➡️📜 looks great. Thanks for this!

paulirish · 2016-11-29T00:07:47Z

just came across this. Awesome work here @olingern!

olingern · 2016-11-29T19:18:07Z

Thanks, @paulirish. You guys have an awesome initiative and it's a fun project to contribute to!

* Includes error message, stack trace, chrome and lighthouse versions in issue body

olingern mentioned this pull request Nov 15, 2016

Report bug from extension popup #919

Closed

add report error button

5f5f9f7

brendankenny requested changes Nov 15, 2016

View reviewed changes

ebidel reviewed Nov 15, 2016

View reviewed changes

olingern added 3 commits November 16, 2016 01:12

update text to textContext, rearrange conditional

0a3bdaf

Add stacktrace, chrome and lighthouse version to link.

286af63

conditional styling for feedback area

33015d2

brendankenny requested changes Nov 16, 2016

View reviewed changes

olingern added 2 commits November 16, 2016 04:07

Place stack trace in code block. Add error message

9029182

fix Lighthouse function casing

e78062d

ebidel reviewed Nov 16, 2016

View reviewed changes

brendankenny approved these changes Nov 16, 2016

View reviewed changes

brendankenny merged commit b118119 into GoogleChrome:master Nov 16, 2016

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017

Extension: add report issue button (GoogleChrome#944)

7172fe2

* Includes error message, stack trace, chrome and lighthouse versions in issue body

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extension: add report error button #944

Extension: add report error button #944

olingern commented Nov 15, 2016 •

edited by brendankenny

Loading

googlebot commented Nov 15, 2016

brendankenny commented Nov 15, 2016

brendankenny left a comment

brendankenny Nov 15, 2016

olingern Nov 16, 2016

ebidel Nov 15, 2016

olingern commented Nov 16, 2016 •

edited

Loading

brendankenny left a comment

brendankenny Nov 16, 2016

olingern Nov 16, 2016

brendankenny Nov 16, 2016

brendankenny Nov 16, 2016

olingern Nov 16, 2016

robdodson Nov 16, 2016

brendankenny Nov 16, 2016

brendankenny Nov 16, 2016

olingern Nov 16, 2016

ebidel commented Nov 16, 2016

@olingern commented on this pull request.

ebidel Nov 16, 2016

olingern Nov 16, 2016

ebidel Nov 16, 2016

brendankenny Nov 16, 2016

brendankenny left a comment

paulirish commented Nov 29, 2016

olingern commented Nov 29, 2016

Extension: add report error button #944

Extension: add report error button #944

Conversation

olingern commented Nov 15, 2016 • edited by brendankenny Loading

googlebot commented Nov 15, 2016

brendankenny commented Nov 15, 2016

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olingern commented Nov 16, 2016 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Nov 16, 2016

@olingern commented on this pull request.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented Nov 29, 2016

olingern commented Nov 29, 2016

olingern commented Nov 15, 2016 •

edited by brendankenny

Loading

olingern commented Nov 16, 2016 •

edited

Loading