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.

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

Improve github issue title #992

Merged
merged 4 commits into from
Nov 22, 2016
Merged

Improve github issue title #992

merged 4 commits into from
Nov 22, 2016

Conversation

olingern
Copy link
Contributor

@olingern olingern commented Nov 21, 2016

Currently the Lighthouse Extension Errors create a generic title that is too ambiguous to differentiate duplicates without actually clicking on the issue.

screen shot 2016-11-21 at 9 58 12 am

This PR appends the JS error to the title.

@@ -53,7 +53,7 @@ document.addEventListener('DOMContentLoaded', _ => {
qsBody += '**Stack Trace**:\n ```' + err.stack + '```';

const base = 'https://github.com/googlechrome/lighthouse/issues/new?';
const title = encodeURI('title=Lighthouse Extension Error');
const title = encodeURI('title=Extension Error: ' + err.message);
const body = '&body=' + encodeURI(qsBody);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this would benefit from some upper bound truncation in case the err.message gets a little long.

Copy link
Contributor Author

@olingern olingern Nov 21, 2016

Choose a reason for hiding this comment

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

Mocking this up in Chrome, Github seems to new line around 77 characters (at my res) per line (60 characters of this error message).

Are two lines okay, or is one line preferable?
screen shot 2016-11-21 at 11 57 00 am

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on truncation. We don't gain much after the first line and the entire message is already in the body.

Copy link
Contributor Author

@olingern olingern Nov 21, 2016

Choose a reason for hiding this comment

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

@addyosmani @ebidel Done, and now this PR truncates appened error messages at 60 characters.

const title = encodeURI('title=Extension Error: ' + err.message);
let titleError = err.message;
if (titleError.length > MAX_ISSUE_ERROR_LENGTH) {
titleError = err.message.substring(0, MAX_ISSUE_ERROR_LENGTH - 3) + '...';
Copy link
Contributor

@ebidel ebidel Nov 21, 2016

Choose a reason for hiding this comment

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

use titleError:

titleError = `${titleError.substring(0, MAX_ISSUE_ERROR_LENGTH - 3)}...`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebidel 👍 concatenation replaced with template literal

@@ -53,7 +55,11 @@ document.addEventListener('DOMContentLoaded', _ => {
qsBody += '**Stack Trace**:\n ```' + err.stack + '```';

const base = 'https://github.com/googlechrome/lighthouse/issues/new?';
const title = encodeURI('title=Lighthouse Extension Error');
let titleError = err.message;
if (titleError.length > MAX_ISSUE_ERROR_LENGTH) {;
Copy link
Member

Choose a reason for hiding this comment

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

delete semicolon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM!

@brendankenny brendankenny merged commit f69ce1c into GoogleChrome:master Nov 22, 2016
@olingern
Copy link
Contributor Author

🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants