Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

fixed notification close bug #311

Merged
merged 2 commits into from Mar 5, 2014

Conversation

Projects
None yet
3 participants
Contributor

jsolis commented Mar 5, 2014

Tiny change on something I noticed wasn't working. I tested this in Chrome and FireFox and it worked fine.

@thedjpetersen thedjpetersen added a commit that referenced this pull request Mar 5, 2014

@thedjpetersen thedjpetersen Merge pull request #311 from jsolis/master
fixed notification close bug
86a16e0

@thedjpetersen thedjpetersen merged commit 86a16e0 into thedjpetersen:master Mar 5, 2014

Owner

thedjpetersen commented Mar 5, 2014

Thanks!

Contributor

totokaka commented on 7d99b98 Mar 5, 2014

This is not needed. The desktopNorification method is bound to the messageNotification event, so it is only called when you are mentioned, se line 35

EDIT: It seems I might have missunderstood what this commit does. If this commit should "fix", the non-existing, issue that the notification is displayed whenever a message is displayed, the original message applies. If it, however, is intended to always display the desktop notification when the user is mentioned, also when the window is active, the correct way would be to just remove the if-statement. Basically this commit does not change functionallity in any way, as it is now.

Contributor

jsolis replied Mar 5, 2014

So my intent was the second case which was to always show the notifications when you are mentioned or PM'ed. The reasoning was that if my tab is in the foreground of my browser, but my browser is in the background, I do not get a notification for a DM or PM but I would want to in this case. I see your point however that the condition should really just be removed in this case. I didn't dig deep enough to realize this when I made this change. When I sent my pull request, this change wasn't intended to be included yet because I had a feeling I wasn't doing it right yet. Ideally this feature would be a user settings option so people can pick how often they get nagged for DM and PMs.

Contributor

totokaka replied Mar 6, 2014

Hmm, I do not believe the per user config will ever be implemented in this branch. Thedjpetersen has started a rewrite, and that will probably contain the per user settings.

The way to fix your problem should either be to remove the if statement, or find a way to check if the browser is in the background.

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