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

Faster desktop notifications #2955

Merged

Conversation

mccambridge
Copy link
Contributor

@RocketChat/core

Adds setting under General -> Notifications to set the number of seconds a desktop notification should appear in the browser (defaults to 5 sec). Currently, Chrome defaults to 20 seconds, and this is somewhat annoying.

Implemented Notification.close() in notifications.coffee in a setTimeout with the duration of the timeout equal to the settings value provided (x1000ms).

@mccambridge mccambridge changed the title Shorter desktop notifications Faster desktop notifications Apr 19, 2016
@rodrigok
Copy link
Member

@mccambridge Are you using OS X? I think that this remove the notification from the notification central.

@mccambridge
Copy link
Contributor Author

mccambridge commented Apr 20, 2016

@rodrigok Yes, I'm on OS X. I don't think these notifications add anything to notification central. These are just pop-up desktop notifications as far as I can tell. The timeout just closes it. https://developer.mozilla.org/en-US/docs/Web/API/notification

@mccambridge
Copy link
Contributor Author

If you'd like to see a fake notification using the same timeout, you can run:

const n = new Notification('lala', { body: 'yada', silent: true}); setTimeout(() => n.close(), 5000)

in a web console.

@rodrigok
Copy link
Member

@mccambridge Yes, the notifications goes to OS X notification center, see #1866 and #1890

As you can see here https://github.com/RocketChat/Rocket.Chat/pull/1890/files, we removed the close timeout to keep notifications in notification center.

@mccambridge
Copy link
Contributor Author

Okay, got it. Personally, I don't use notification center and would rather not see the notification linger for so long. But that's just my opinion.

What do you think about a setting where the administrator could choose whether or not to set the timeout?

@rodrigok
Copy link
Member

@mccambridge an option of notification timeout is a good idea, 0 could be disabled

@mccambridge
Copy link
Contributor Author

@rodrigok okay, updated to include 0 as an option, by which the timeout would not be set. 0 is the default value.

@geekgonecrazy
Copy link
Contributor

@mccambridge So just to confirm when 0 they will behave as they do currently?

@mccambridge
Copy link
Contributor Author

Exactly. And default is 0, so default is they behave as they do currently.

@rodrigok
Copy link
Member

LGTM

@rodrigok
Copy link
Member

@RocketChat/core Someone can review to give the second lgtm?

@sampaiodiego
Copy link
Member

LGTM

@engelgabriel engelgabriel merged commit f4693f1 into RocketChat:develop Apr 20, 2016
@mccambridge
Copy link
Contributor Author

Thanks, y'all!

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.

5 participants