[Android] Add the ability to remove notifications based on the tag #1058
Conversation
There was a problem hiding this comment.
@dgruseck big thanks for this, sorry it's taken a little while to review properly. I've suggested a couple of minor changes - if you're able to make these I can get it merged in for the next release.
lib/modules/notifications/index.js
Outdated
| * Remove a delivered notifications by tag. | ||
| * @param tag | ||
| */ | ||
| removeDeliveredNotificationsByTag(tag: string): Promise<void> { |
There was a problem hiding this comment.
Are you able to move this into AndroidNotifications.js as it's Android specific functionality?
You'll see it that file, that the JS does a platform check when a method is called and resolves an empty promise if it's not Android. If we follow that pattern then we don't need to worry about the iOS implementation that you've added.
| androidMap.putMap("smallIcon", iconMap); | ||
| } | ||
| if (notification.getTag() != null) { | ||
| androidMap.putString("group", notification.getTag()); |
There was a problem hiding this comment.
Now that we have the tag property, I don't think we should also set that as the group as they are two separate things. Are you able to remove the group?
There was a problem hiding this comment.
Removing the group could be a breaking change for some people, because of that I'm not sure if we should do that.
|
This would be great, or even just the part which adds the ability to call setTag when creating a local notification (seems like quite an inconsistency between local/remote notifications). |
Salakar
added
Workflow: Waiting for User Response
|
@dgruseck any updates on the requested changes? |
Salakar
removed
the
Workflow: Waiting for User Response
|
@dgruseck thanks for updating this, looks fine to me now. Will merge but could you update the docs to add these on please; https://rnfirebase.io/docs/master/notifications/reference/AndroidNotification Loving
|
4 participants
It would be awesome to be able to remove notification using the tag.