notification issue fixed #240

mebjas · 2016-03-21T22:25:47Z

Signed-off-by: Minhaz A V minhazav@gmail.com

Changes

isPermissionDefault(): added a method to check id the current notification preference is default.
showToolbar(): added a method that returns bool value corresponding to whether or not we should show the toolbar. It first checks localStorage if the user has clicked on close button of the toolbar, then checks if the status of notification permission isn't default.
setEnabled (): now uses the then() method of Notification class to deal with user response to activate notification dialog. Performs necessary actions as per user's response. No changes if the user simply dismisses the dialog.
setToolbarHidden (): added an extra parameter that defines whether to store the setting persistently or not.
setToolbarPersistantHidden (): added to store the setting to localStorage

Signed-off-by: Minhaz A V <minhazav@gmail.com>

richvdh · 2016-03-22T10:57:22Z

src/Notifier.js

+        return global.Notification.permission == 'default';
+    },
+
+    showToolbar: function() {


This doesn't seem to be used anywhere. Is there another PR to come?

showToolbar sounds like it will show the toolbar. This should be shouldShowToolbar or similar (and it needs a comment saying what it does)

It's used by client to know weather or not to show the toolbar, I'll change it to shouldShowToolbar() though
element-hq/element-web#1237

Thanks. Sorry I missed the other PR. It's useful to say something about the other PR in the PR comments!

richvdh · 2016-03-22T11:17:01Z

For the record: this relates to element-hq/element-web#1095 and element-hq/element-web#951

richvdh · 2016-03-22T11:21:29Z

@mebjas: Thanks very much for the PR. Generally it looks good, though maybe a bit more complicated than I expected. I've made a few comments which might also help simplify things a bit.

Signed-off-by: Minhaz A V <minhazav@gmail.com>

mebjas · 2016-03-23T08:18:52Z

src/Notifier.js

+            if (this.isPermissionDefault()) {
+                // Attempt to get permission from user
+                var self = this;
+                global.Notification.requestPermission().then(function(result) {


So I shall shift this to callback one? I think that MDN doc is little updated, as this is supported in chrome now.

Yes, I think you might as well use the callback flow.

Obviously the MDN doc is outdated, but we don't know which version of Chrome introduced the promise; we don't know if it works in Safari; and using the promise doesn't buy us anything.

Signed-off-by: Minhaz A V <minhazav@gmail.com>

mebjas · 2016-03-23T08:31:18Z

src/Notifier.js

+
+        // means the permission is blocked
+        if (!this.isPermissionDefault()) return false;
+        return true;


@richvdh Tell me what do you think about showing a message if the user has blocked notif request & not yet attempted to close the toolbar - if yes, I'll change this method to return enums - 'show', 'hide', 'blocked' and the vector will take decision accordingly.

Signed-off-by: Minhaz A V <minhazav@gmail.com>

richvdh · 2016-03-23T10:04:43Z

src/Notifier.js

+        }
+    },
+
+    setToolbarPersistantHidden: function() {


still needs inlining

Signed-off-by: Minhaz A V <minhazav@gmail.com>

richvdh · 2016-03-23T10:24:30Z

src/components/structures/MatrixChat.js

@@ -1046,7 +1046,7 @@ module.exports = React.createClass({
            if (MatrixClientPeg.get().isGuest()) {
                topBar = <GuestWarningBar />;
            }
-            else if (Notifier.supportsDesktopNotifications() && !Notifier.isEnabled() && !Notifier.isToolbarHidden()) {
+            else if (Notifier.supportsDesktopNotifications() && Notifier.shouldShowToolbar()) {


I don't think this change is necessary. It's clearer to keep isEnabled and isToolbarHidden separate imho

richvdh · 2016-03-23T10:34:28Z

@mebjas: better, but still quite a lot of the issues we discussed remain!

mebjas · 2016-03-23T11:41:05Z

@richvdh why are we keeping notifications_enabled in localStorage at all?
And not rely on Notification.permission ?

richvdh · 2016-03-23T15:08:18Z

We might have permission from the browser to show notifications, but the user has disabled the setting in Vector. That is what notifications_enabled is about.

richvdh · 2016-03-23T16:27:07Z

src/Notifier.js

-            global.localStorage.setItem('notifications_enabled', 'true');
-
-            if (this.havePermission) {
+            // Case when we do not have the permission as 'granted'


This comment is no longer helpful

richvdh · 2016-03-23T16:35:46Z

src/Notifier.js

+                    return;
+                }
+
+                if (callback) callback();


I think you need to update the localStorage setting before you run the callback and dispatch the notifier_enabled setting, otherwise things will get confused.

richvdh · 2016-03-23T16:36:28Z

Much better :). Nearly there :)

Signed-off-by: Minhaz A V <minhazav@gmail.com>

richvdh · 2016-03-23T23:18:59Z

This is looking really good now. I think there is one remaining problem though: Once you dismiss the banner, it will never, ever come back again. Suppose:

I don't want to enable notifications so I dismiss the banner
Later I decide I do want notifications after all so I enable them via the settings dialog
Later still I turn them off again.
At this point I think it makes sense if we show the banner again.

My suggestion would be that you call setToolbarHidden(false) from setEnabled if enabled is true.

Signed-off-by: Minhaz A V <minhazav@gmail.com>

mebjas · 2016-03-25T08:54:14Z

@richvdh ?

richvdh · 2016-03-29T09:47:17Z

looks good, thank you!

notification issue fixed

98343a0

Signed-off-by: Minhaz A V <minhazav@gmail.com>

richvdh reviewed Mar 22, 2016
View reviewed changes

Some style fixes

1875377

Signed-off-by: Minhaz A V <minhazav@gmail.com>

mebjas reviewed Mar 23, 2016
View reviewed changes

persistent = true, as default param in setToolbarHidden() method

818299d

Signed-off-by: Minhaz A V <minhazav@gmail.com>

mebjas reviewed Mar 23, 2016
View reviewed changes

mebjas added 2 commits March 23, 2016 15:11

funciton name changed to

cbcf23f

Signed-off-by: Minhaz A V <minhazav@gmail.com>

logic to show/hide toolbar shifted to MatrixChat::render

349b472

Signed-off-by: Minhaz A V <minhazav@gmail.com>

richvdh reviewed Mar 23, 2016
View reviewed changes

src/Notifier.js

}

},

setToolbarPersistantHidden: function() {

Copy link

Member

richvdh Mar 23, 2016

Choose a reason for hiding this comment

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

still needs inlining

promise -> callback & setToolbarPersistantHidden moved inline

9b5519e

Signed-off-by: Minhaz A V <minhazav@gmail.com>

richvdh reviewed Mar 23, 2016
View reviewed changes

unwanted functions removed, prev functions modified

8191eaa

richvdh reviewed Mar 23, 2016
View reviewed changes

comment removed and localStorage update moved up

8605ca1

Signed-off-by: Minhaz A V <minhazav@gmail.com>

setToolbarHidden(false) if setEnabled has enable = true

6fc0aae

Signed-off-by: Minhaz A V <minhazav@gmail.com>

richvdh merged commit d7f8017 into matrix-org:develop Mar 29, 2016

This was referenced Mar 29, 2016

Persist the 'you are not receiving notifs' warning state in account user_data element-hq/element-web#951

Closed

Desktop notifs prompt doesn't disappear when you click enable element-hq/element-web#1095

Closed

mebjas deleted the develop-notif branch March 29, 2016 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

notification issue fixed #240

notification issue fixed #240

mebjas commented Mar 21, 2016

richvdh Mar 22, 2016

mebjas Mar 22, 2016

richvdh Mar 22, 2016

richvdh commented Mar 22, 2016

richvdh commented Mar 22, 2016

mebjas Mar 23, 2016

richvdh Mar 23, 2016

mebjas Mar 23, 2016

richvdh Mar 23, 2016

richvdh Mar 23, 2016

richvdh commented Mar 23, 2016

mebjas commented Mar 23, 2016

richvdh commented Mar 23, 2016

richvdh Mar 23, 2016

richvdh Mar 23, 2016

richvdh commented Mar 23, 2016

richvdh commented Mar 23, 2016

mebjas commented Mar 25, 2016

richvdh commented Mar 29, 2016

notification issue fixed #240

notification issue fixed #240

Conversation

mebjas commented Mar 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Mar 22, 2016

richvdh commented Mar 22, 2016

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

richvdh commented Mar 23, 2016

mebjas commented Mar 23, 2016

richvdh commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Mar 23, 2016

richvdh commented Mar 23, 2016

mebjas commented Mar 25, 2016

richvdh commented Mar 29, 2016