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

Make reload desktop notifications more concise #796

Closed
kumar303 opened this issue Feb 9, 2017 · 6 comments · Fixed by #815
Closed

Make reload desktop notifications more concise #796

kumar303 opened this issue Feb 9, 2017 · 6 comments · Fixed by #815

Comments

@kumar303
Copy link
Contributor

kumar303 commented Feb 9, 2017

Is this a bug or feature request?

feature

What is the current behavior?

  • change into an extension source directory
  • type web-ext run
  • edit one of the source files to create a reload error. For example: delete one of the required fields from your manifest.

You will see a desktop notification telling you that the extension could not be reloaded because of an error:
web-ext-reload

What is the expected or desired behavior?

It's hard to glance at this and know what just happened. Can we make it more concise? Here is a suggestion (but maybe there are other things we can do):

Change the body to "unknownError: error occurred while processing ...." In other words, remove "reload response error:" from the message because that's redundant. The title of the notification already tells you that an error occurred.

Version information (for bug reports)

  • Firefox version:
  • Your OS and version:
  • Paste the output of these commands:
node --version && npm --version && web-ext --version

v6.9.4
3.10.10
1.8.1

@kumar303 kumar303 changed the title Make reload desktop notification more concise Make reload desktop notifications more concise Feb 9, 2017
@saintsebastian
Copy link
Contributor

this is indeed a pretty good first bug! I hope someone will pick it up soon as an introduction to web-ext

@kumar303
Copy link
Contributor Author

kumar303 commented Feb 9, 2017

@saintsebastian If you want to grab it, then feel free :) I only added the label since it is a small change but I do think we should make this change sooner rather than later.

@francesar
Copy link

@kumar303 Hi! If no one has taken this on yet, I'd love to take a stab at it. (with some guidance since this would be my first open source bug fix)

@saintsebastian
Copy link
Contributor

saintsebastian commented Feb 13, 2017

@francesar Hey! I think I can point you to a couple of places to look into. Notifications utility function is here and they are created here. Let me know if I can help you with anything else and good luck!

@0xst4n
Copy link
Contributor

0xst4n commented Feb 20, 2017

Hey, I am new to open source, and I think I fixed this issue (I'm not actually sure if I did it correctly) Is it okay if I open a pull request?

@wagnerand
Copy link
Member

@kumar303 would you mind mentoring this?

@Standb Absolutely! Please open a PR.

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

Successfully merging a pull request may close this issue.

5 participants