-
-
Notifications
You must be signed in to change notification settings - Fork 310
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
Add setting to disable the update available notification (#1) #1757
Conversation
* Add setting to disable the update available notification
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.
Awesome stuff! 🎉 Changes look good to me. Thanks for putting this together. I just added one small comment.
.subscribe((disableUpdateNotification: boolean) => { | ||
if (!disableUpdateNotification) { | ||
this.ipc.on('update-available', () => { | ||
this.notifyService.info('Found new updates!, go to Update to get the latest version', 'Update Found!') |
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.
How do I go to Update?
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.
Perhaps clicking the notification should have the same effect. i.e. clicking the notification should trigger the update.
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.
Also just looked at the screenshots. I'm concerned we now have both "Update" and "Check for updates.." menu items. That can get confusing.
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.
Yep, it's confusing, clicking the notification to get the update seems to be a better idea, if u are ok with that i'll make the change in no time :D
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.
I've made the change, now there's no "update" option next to "check for updates" and when you click on the notification it downloads the update
* Add setting to disable the update available notification * Changes setting text * lint fix * Add download update on click and remove update option from menu * Removes update option
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.
Amazing work! Just left one tiny comment. I could also simply change that myself if you want. Thanks!
Fixes
#1612
Checks
yarn test-build
Changes proposed in this pull request: