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

new value 'all' for 'show_desktop_notifications' #1305

Merged
merged 1 commit into from Nov 6, 2018

Conversation

@ChaosKid42
Copy link
Contributor

commented Nov 4, 2018

I propose that we add an option notify_in_view which defaults to false. Enabling it would make converse.js show notifications even if the tab of converse.js is shown to the user.

@ChaosKid42 ChaosKid42 changed the title add option `notify_in_view` add option "notify_in_view" Nov 4, 2018

@ChaosKid42 ChaosKid42 force-pushed the ChaosKid42:notify_in_view branch from df83fa7 to 3af914b Nov 4, 2018

@jcbrand

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

I'd like us to reuse existing config settings whenever possible.

So instead of having a new option, I suggest we re-use show-desktop-notifications for this.

It can then have the following values:

  • true or 'hidden' which is the current functionality.
  • false or null or '', which disables desktop notifications.
  • 'all' which would be like setting notify_in_view to true.

This would be a backwards compatible way of adding your desired feature without adding a new config option.

@ChaosKid42 ChaosKid42 force-pushed the ChaosKid42:notify_in_view branch from 3af914b to 14654ec Nov 5, 2018

@ChaosKid42

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

@jcbrand Another great idea. PR updated.

@ChaosKid42 ChaosKid42 force-pushed the ChaosKid42:notify_in_view branch from 14654ec to 9e53e43 Nov 5, 2018

@ChaosKid42 ChaosKid42 changed the title add option "notify_in_view" new value 'all' for 'show_desktop_notifications' Nov 5, 2018

@ChaosKid42 ChaosKid42 force-pushed the ChaosKid42:notify_in_view branch from 7c026e1 to e970d2d Nov 6, 2018

@ChaosKid42

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

@jcbrand: I rebased the commit. Should be bossible to merge it now.

@jcbrand jcbrand merged commit 8ba8a4b into conversejs:master Nov 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jcbrand

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

Thanks!

@ChaosKid42 ChaosKid42 deleted the ChaosKid42:notify_in_view branch Feb 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.