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(android): add group and groupSummary to LocalNotifications #2385

Merged

Conversation

p7g
Copy link
Contributor

@p7g p7g commented Jan 29, 2020

This PR adds group and groupSummary options in LocalNotifications.schedule(), which allows you to group notifications together on Android.

I noticed a TODO comment in LocalNotificationManager.Build for groups, which says to implement setGroup, setGroupSummary, and setNumber. This PR only implements the first 2, should I also add setNumber?

@jcesarmobile
Copy link
Member

I'm not able to make this work, can you maybe provide a sample app where I can easily test it?

Also, core-plugin-definitions.ts have now conflicts (probably because I merged your iOS PR). Can you fix it and also document that those are Android only and a brief description (for the 4 new options)

@p7g p7g force-pushed the android-localnotifications-group branch from b7b88ab to a27a52d Compare January 31, 2020 21:15
@p7g p7g force-pushed the android-localnotifications-group branch from a27a52d to ef7134a Compare January 31, 2020 22:07
@p7g
Copy link
Contributor Author

p7g commented Jan 31, 2020

@jcesarmobile Here is a demo app I threw together: https://github.com/p7g/capacitor-android-group-demo

Just run it in the android emulator, send some notifications with a group name, and then send a groupSummary notification with the same group (I put a text box and some buttons on the page). That's essentially the use-case I have.

@jcesarmobile
Copy link
Member

Thanks for the sample app, I got it working.

You have removed group and groupSummary from PushNotification types, but you are putting them in getDeilveredNotifications in PushNotifications.java. getDeilveredNotifications returns a PushNotification, so they types should have group and groupSummary, or make getDeilveredNotifications not return the group and groupSummary

@p7g
Copy link
Contributor Author

p7g commented Feb 3, 2020

Whoops, didn't mean to remove those, I've added them back.

@jcesarmobile
Copy link
Member

can you also add the Android only comments as on the LocalNotification ones?

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.

Looks good!
And thanks for making all the changes

@jcesarmobile jcesarmobile changed the title Android LocalNotifications group and groupSummary feat(android): add group and groupSummary to LocalNotifications Feb 3, 2020
@jcesarmobile jcesarmobile merged commit 8e8a157 into ionic-team:master Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants