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

Update receiving-messages.md #242

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VilleMiekkoja
Copy link

Improve docs; Request permissions section only relevant for iOS. On android, it always resolves, and has no use or effect on anything.

@mikehardy
Copy link
Collaborator

I also fear it may have changed with Android 10 - I'm not sure. Have you consulted the docs on react-native-permissions? Looks like it may be possible for Notifications to be disabled in Android under some cases:

https://github.com/react-native-community/react-native-permissions/blob/master/android/src/main/java/com/reactnativecommunity/rnpermissions/RNPermissionsModule.java#L89

So even though in v5 of react-native-firebase it always resolves to true, in reality now (after v5 here was written, and is now orphaned) it is a good idea to check.

For that reason I suggested adding a pointer to the 'react-native-permissions' library and mentioning it should be used prior to calling requestPermissions here, to make sure you really are handling permissions correctly

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

add section indicating react-native-permissions should be consulted prior to this call now

@gianpaj
Copy link
Contributor

gianpaj commented Feb 6, 2020

I strongly suggest adding this with Mike's addition. I was getting an FCM push token (somehow) but the app was still not registering for Push Notifications with iOS (no Notifications access under the Settings -> App).

After adding react-native-permissions and its requestNotifications() method the app finally prompted of the access 🎉

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

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.

4 participants