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.

Sign up for GitHub

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

Jump to bottom

feat(PushNotifications): Make register method to return if permission was granted #2324

Merged
merged 1 commit into from
Feb 4, 2020
Merged

feat(PushNotifications): Make register method to return if permission was granted #2324

merged 1 commit into from
Feb 4, 2020

Conversation

chrisweight
Copy link
Contributor

@chrisweight chrisweight commented Jan 13, 2020 •
edited
Loading

Rather than simply calling call.success() on iOS as soon as a user requests permissions (regardless of result), this adds a completion that allows the user to await the result in their app and passes back the response or error.

@chrisweight
Copy link
Contributor Author

chrisweight commented Jan 14, 2020 •
edited
Loading

NOTE: This conflicts with the proposed approach in #2249.

Preferring the approach codified here as opposed to ^^ as it allows a consuming app to simply 'await' the response as per the standard approach documented in most Capacitor native APIs, rather than having to register / maintain event listeners.

@corysmc
Copy link

corysmc commented Jan 16, 2020

For what it's worth, I'd prefer this method as well ☝️

jcesarmobile
jcesarmobile requested changes Jan 17, 2020
View reviewed changes
Copy link
Member

@jcesarmobile jcesarmobile left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the unregister functionality to a separate PR?

And it would need to be added in the types and implemented for Android too.

And about the register change, the same, the types should be updated too, and in Android, as it doesn't ask for permission, I think it should just always return granted.

@chrisweight chrisweight changed the title [iOS] Adds completion to iOS PN permissions request, adds an unregister() method [PushNotificationPlugin] Adds completion to iOS PN permissions request, adds 'granted' return bool Jan 20, 2020
@chrisweight
Copy link
Contributor Author

Can you move the unregister functionality to a separate PR?

And it would need to be added in the types and implemented for Android too.

And about the register change, the same, the types should be updated too, and in Android, as it doesn't ask for permission, I think it should just always return granted.

@jcesarmobile - Ok, unregister removed, Android, types and docs updated....

@chrisweight
Copy link
Contributor Author

@jcesarmobile - Anything else you want changing on this, or is this good to go?

@jcesarmobile
Copy link
Member

Since it changes the API it's a breaking change, so won't be merging until I start working on Capacitor 2.0

You can send a separate PR with the unregister implementation, that could be merged before 2.0 since it's a new API

@jcesarmobile jcesarmobile added this to the 2.0.0 milestone Jan 22, 2020
@jcesarmobile jcesarmobile mentioned this pull request Jan 29, 2020
Closed
jcesarmobile
jcesarmobile approved these changes Feb 4, 2020
View reviewed changes
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, thanks!

@jcesarmobile jcesarmobile changed the title [PushNotificationPlugin] Adds completion to iOS PN permissions request, adds 'granted' return bool feat(PushNotifications): Make register method to return if permission was granted Feb 4, 2020
@jcesarmobile jcesarmobile merged commit a0bcf5c into ionic-team:master Feb 4, 2020
chrisweight added a commit to chrisweight/capacitor that referenced this pull request Feb 17, 2020
@chrisweight
Merge remote-tracking branch 'upstream/master'
de72bcd 
* upstream/master: (50 commits)
  feat(Device): Add getBatteryInfo function (ionic-team#2435)
  fix(doctor): add electron checks (ionic-team#2434)
  chore(android): target SDK version 29 (ionic-team#2433)
  feat(android): update gradle and dependencies (ionic-team#2431)
  fix(ios): Make Clipboard plugin return errors (ionic-team#2430)
  feat(android): Handle onDestroy lifecycle event in plugins (ionic-team#2421)
  docs(ce-plugins): Remove or replace deprecated plugins (ionic-team#2419)
  feat(ios): change native location accuracy values (ionic-team#2420)
  docs(ce-guides): Remove dead link (ionic-team#2418)
  docs(network): Remove example guide because of dead link (ionic-team#2417)
  feat(electron): Remove injectCapacitor function (ionic-team#2415)
  fix(android): plugin retained events not being retained if listeners were empty (ionic-team#2408)
  chore(android): remove unused launch_splash.xml (ionic-team#2411)
  feat(Filesystem): Remove createIntermediateDirectories from MkdirOptions (ionic-team#2410)
  feat(android): use Fused Location Provider on Geolocation plugin (ionic-team#2409)
  fix(toast): unify duration across platforms (ionic-team#2340)
  feat(PushNotifications): Make register method return if permission was granted (ionic-team#2324)
  chore(circleci): update Xcode and remove install-cocoapods job (ionic-team#2402)
  chore(cli): fix tests for newer node versions (ionic-team#2403)
  fix(android): return original camera image if edition was canceled (ionic-team#2358)
  ...
@jcesarmobile
Copy link
Member

I've added a new requestPermission method and that's the one returning granted now, as the PR also changes register method to just call UIApplication.shared.registerForRemoteNotifications() and not prompt.

#2516

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcesarmobile jcesarmobile jcesarmobile approved these changes

Assignees
No one assigned
Labels
priority: high
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@chrisweight @corysmc @jcesarmobile