Browser notifications #284
Conversation
documentation update
documentation update
Codecov Report
@@ Coverage Diff @@
## master #284 +/- ##
==========================================
- Coverage 71.65% 70.77% -0.88%
==========================================
Files 9 9
Lines 515 527 +12
Branches 107 110 +3
==========================================
+ Hits 369 373 +4
- Misses 146 154 +8 |
|
@oktapodia and maybe also this one, we would like this feature :) |
There was a problem hiding this comment.
Hi @frankdekker, this looks like a great feature and it also works on localhost w/out https which should make it very useful for lots of people. I'd like to make a couple changes to how support is communicated to the user as well as disabling. if you have other thoughts on this, please share them!
|
|
||
| app.controller('MainCtrl', [ | ||
| '$scope', '$rootScope', '$http', 'Email', '$route', '$location', 'Favicon', | ||
| function ($scope, $rootScope, $http, Email, $route, $location, Favicon) { | ||
| $scope.items = [] | ||
| $scope.configOpen = false | ||
| $scope.currentItemId = null | ||
| $scope.notificationsSupported = 'Notification' in window |
There was a problem hiding this comment.
I think we should give user feedback if it's supported also using isSecureContext.
There was a problem hiding this comment.
Interesting, what kind of feedback are you thinking of?
There was a problem hiding this comment.
I decided to add a title attribute on the Notifications toggle now. Also with the new toggle design, Notifications is now greyed out if the browser context does not support it:
| @@ -93,6 +108,27 @@ app.controller('MainCtrl', [ | |||
| $scope.autoShow = !$scope.autoShow | |||
| } | |||
|
|
|||
| $scope.enableNotifications = function () { | |||
| if (window.Notification && window.Notification.permission === 'granted') { | |||
| window.alert('To disable notifications, revoke the permissions in your browser.') | |||
There was a problem hiding this comment.
Instead of this alert, can we instead set $scope.webNotifications = false? This context will of course be lost when the browser window is closed, but it might be a better workflow. Perhaps if this was coupled with some setting storage in localStorage that may provide the best experience.
There was a problem hiding this comment.
Interesting, hadn't thought about that. So instead of revoking permissions, you just silence the notifications. Great idea. The silence setting has to be stored preferrably. I as user found it already annoying to toggle the "autoShow" each time.
There was a problem hiding this comment.
Thanks for the input - all settings will be persisted after #291 :)
| @@ -76,9 +79,15 @@ module.exports = function (config) { | |||
| } | |||
|
|
|||
| if (!config.disableWeb) { | |||
| const secure = { | |||
There was a problem hiding this comment.
I like this grouping of the options together. Makes me think that web.start()'s arguments should be refactored soon.
| window.alert('Unable to enable web notifications. See console for more information') | ||
| }) | ||
| if (!window.isSecureContext && window.console) { | ||
| console.info( |
There was a problem hiding this comment.
I'd rather make this more obvious to the user, perhaps using window.alert or a title on the html element.
| @@ -0,0 +1,32 @@ | |||
| # HTTPS | |||
| By default MailDev will run on the http protocol. For Web Notification support you'll need | |||
There was a problem hiding this comment.
One can also use web notification support on localhost in most browsers. We should update this copy.
There was a problem hiding this comment.
Ah. I wasn't aware localhost was the exception. Indeed useful to be added.
|
|
||
| ## Open maildev with https | ||
| ``` | ||
| https:\\192.168.1.103:1080 |
There was a problem hiding this comment.
| https:\\192.168.1.103:1080 | |
| https:\\192.168.1.103:1080 | |
| https://192.168.1.103:1080 |
Added notification support to the Web Interface.
As Chrome/Firefox require secure context to allow notifications, added https cli arguments
See docs/https.md for details.