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

feat(LocalNotifications): add createChannel, deleteChannel and listChannels methods #2676

Merged
merged 6 commits into from
Apr 3, 2020
Merged

feat(LocalNotifications): add createChannel, deleteChannel and listChannels methods #2676

merged 6 commits into from
Apr 3, 2020

Conversation

dwlrathod
Copy link
Contributor

@dwlrathod dwlrathod commented Apr 1, 2020

Starting in Android 8.0 (API level 26), all notifications must be assigned to a channel. For each channel, you can set the visual and auditory behaviour that is applied to all notifications in that channel. Then, users can change these settings and decide which notification channels from your app should be intrusive or visible at all.

This PR is for allowing channel feature to LocalNotification. The first user needs to create a Notification channel with createChannel method and then assign channelId at the time of scheduling notification. If no channelId is provided will use the default channel and if provided channelId does not exist, the notification will not be generated.
Fixes #2675

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

First of all, when you create a feature request (issue or PR, you should provide a lot more information about what you want, why is good, how to use, etc.), this PR has no information at all other than a link to an issue with little information on its description.

By looking at the code, you are removing the default channel, I don't think that's good, one thing is allowing to use different channels, and another is to force using a channel. The channelId should be optional and use the default channel if an id was not provided. If not, this will break all existing apps.

The method on the manager should be listChannels, not listChannel

Also, you should add the methods for iOS too despite they won't work, they should do the same they do on PushNotifications plugin.

About the web, put the imports on top, not in the methods.

Finally, on the types, we put the regular methods before the addListener methods, you have put them after the removeAllListeners, can you move them up?

@dwlrathod
Copy link
Contributor Author

dwlrathod commented Apr 2, 2020

@jcesarmobile good points. Hopefully, all are done.

Thank you for your time and help! 🙂

@jcesarmobile jcesarmobile changed the title LocalNotifiaction multiple channels fixes #2675 feat(LocalNotification): add createChannel, deleteChannel and listChannels methods Apr 3, 2020
@jcesarmobile jcesarmobile changed the title feat(LocalNotification): add createChannel, deleteChannel and listChannels methods feat(LocalNotifications): add createChannel, deleteChannel and listChannels methods Apr 3, 2020
@ionic-team ionic-team deleted a comment from dwlrathod Apr 3, 2020
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

I've made a few changes, will merge once the tests finish.

@jcesarmobile jcesarmobile merged commit d72e25d into ionic-team:master Apr 3, 2020
dibyendusaha added a commit to dibyendusaha/capacitor that referenced this pull request Apr 10, 2020
…ATION instead of USAGE_ALARM, to sync volume control as per notification volume
dibyendusaha added a commit to dibyendusaha/capacitor that referenced this pull request Apr 10, 2020
…USAGE_NOTIFICATION instead of USAGE_ALARM, as it will give control to device notification volume instead of alarm
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.

feat(LocalNotification): multiple notificaiton channel for Android
2 participants