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

use device defaults for notification features #283

Merged
merged 1 commit into from
Apr 14, 2017

Conversation

grendell
Copy link

@grendell grendell commented Nov 3, 2015

This includes sound, lights, and vibration settings.

Without this, Notification.defaults defaults to 0 while Notification.DEFAULT_ALL == -1, which will result in no vibration or sound for any notification, even when the user has set these to happen.

More info here.

@grendell
Copy link
Author

grendell commented Dec 1, 2015

+2

Seriously though, please let me know if there's anything I can do to help get this merged. Thanks.

@yinfeiru
Copy link
Contributor

yinfeiru commented Feb 2, 2016

@grendell Thanks for the PR! Although what you suggested makes perfect sense, I'd like to add a flag in MPConfig (eg. https://github.com/mixpanel/mixpanel-android/blob/master/src/main/java/com/mixpanel/android/mpmetrics/MPConfig.java#L176) so that people will be able to turn this feature on/off. A lot of existing users have AutoShowMixpanelUpdates on and notifications will be shown when the app is resumed. In that case, since the app is in foreground already, there's no need to alert the user.

@grendell
Copy link
Author

grendell commented Feb 8, 2016

@yinfeiru thanks for the feedback. I have updated the pull request accordingly.

@NickMele
Copy link

Is there a plan/timeline to get this PR merged?

@patedit patedit merged commit a9be5bd into mixpanel:master Apr 14, 2017
@patedit
Copy link
Contributor

patedit commented Apr 14, 2017

Hi guys, I've modified the PR since it had some conflicts and merge it to master. Thanks for your contribution!

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.

4 participants