-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Adds support for badging on iOS #740
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
Conversation
👍 FWIW |
7a19ee5
to
aea9c87
Compare
@flovilmart updated the pull request. |
aea9c87
to
ee3b37d
Compare
@flovilmart updated the pull request. |
return badgeUpdate.then(() => { | ||
return rest.find(config, auth, '_Installation', where) | ||
}).then((response) => { | ||
if (body.badge && body.badge == "Increment") { |
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.
This looks fine, what do you think if we would inplace mutate every installation instead before sending?
Which looks like you already sort of do with collection iteration aka results.reduce
, but this would probably reduce complexity of reading this...
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.
Also, would actually remove the requirement to send multiple times, since the complexity of sending with a badge and without is the same.
Optimization suggestion, feel free to ignore though. |
Adds support for badging on iOS
rest.find(config, auth, '_Installation', where).then(function(response) { | ||
let badgeUpdate = Promise.resolve(); | ||
|
||
if (body.badge) { |
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.
is there a way to reuse this code for Amazon SNS?
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.
You actually don't need to use this piece, since this happens before PushAdapter, which is the extension point where SNS would be plugged in.
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.
Even better...thanks!
Fix for #723