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

Support for localized alert push notifications (title-loc-key, loc-key, title-loc-args etc) #36

Closed
wants to merge 2 commits into from

Conversation

btrappe
Copy link

@btrappe btrappe commented Sep 22, 2020

Great project it helped me very much to shorten my dev time !
Beside the loc features I added SetToken function because I need to send a personalized message to a number of devices of the same user, thus it is convenient to prepare a push and then simple loop through the device token list.

@alexalok
Copy link
Owner

Hello Bernd,

thank your for your work! Sorry for not reviewing it for so long. I still feel a bit conflicted about the SetToken method because I'd rather keep the ApplePush object non-reusable (that is, you create it with the specific parameters to the specific sender, you send it and then throw it away). As for your use-case, I think it's better to allow providing multiple push tokens to a single push. I've fired a separate issue for that.

If possible, could you extract the SetToken-related commit to another branch and leave only the localized alert related one in this pull request?

It would be perfect to have some tests as well, but I can write them myself if you don't have time for that currently.

@mtalaga
Copy link
Contributor

mtalaga commented Jan 7, 2021

Hi Alex, will this PR be merged sometime soon? I need to use loc-key and loc-args approach for sending push messages, and this would be useful for me. If not, is there any other way how can I achieve that without merging this PR?

@alexalok
Copy link
Owner

alexalok commented Jan 7, 2021

Hi, @mtalaga!

There are 2 issues preventing this PR from merging:

  1. It introduces SetToken method, which is not related to the PR itself.
  2. It lacks tests for the new AddAlert overload.

Unfortunately, I won't have time to work with that in the nearest future. If you feel brave, you can fork the repo and create a PR yourself.

@alexalok
Copy link
Owner

alexalok commented Feb 2, 2021

Superseded by #62.

@alexalok alexalok closed this Feb 2, 2021
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.

3 participants