-
Notifications
You must be signed in to change notification settings - Fork 614
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
fix(push-notifications): use id and tag for canceling active notification #1041
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since getDeliveredNotifications now returns the notification tag, the types should be edited to reflect that, the tag should be optional and documented that it's only returned for Android push notifications (getDeliveredNotifications also returns local notifications, but they don't usually have a tag)
...android/src/main/java/com/capacitorjs/plugins/pushnotifications/PushNotificationsPlugin.java
Show resolved
Hide resolved
* | ||
* Only available on Android (from push notifications). | ||
* | ||
* @since 1.0.9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @since 1.0.9 | |
* @since 4.0.0 |
This will be available on version 4.0.0 of the plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment.
Also, you have to re run npm run build
to update the README
Integer id = notif.getInteger("id"); | ||
ids.add(id); | ||
String tag = notif.getString("tag"); | ||
Integer id = notif.getInteger("id", 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The id is not optional, so you shouldn't need the default value.
Have you seen it being null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, I removed it.
Fixes an issue where it was not possible to delete individual notifications on Android.
closes: #740