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

[Android] Fixed foreground FCM banner notifications and notification sound #1042

Closed
wants to merge 4 commits into from

Conversation

tankers746
Copy link

No description provided.

evaldsurtans added a commit to evaldsurtans/react-native-push-notification that referenced this pull request May 7, 2019
@zabojad
Copy link

zabojad commented May 28, 2019

@tankers746 I tried your PR and while it fixes custom sounds on Android, it also has a side effect: local notification does not show up at the top of the screen anymore when triggered (they stack in the notification center quietly).

@zabojad
Copy link

zabojad commented May 28, 2019

@tankers746 OK, I have found why but it appears to be another existing bug that was leading to this side effect. The notification importance param value was not passed to the notification channel creation. I've fixed it on my fork there : zabojad@286e7d3

This issue was showing up with you fix because your code creates a new channel when using a custom sound.

@TylerWardle
Copy link

@zabojad any chance you'd be putting in a PR of your fork?

@tankers746
Copy link
Author

@zabojad I've updated the PR with that fix

jnurkka added a commit to jnurkka/react-native-push-notification that referenced this pull request Oct 24, 2019
@HarshitMadhav
Copy link

This commit is not merged to master.

@iamshadmirza
Copy link

Please merge this PR to master

@dabakovich
Copy link

@zo0r please check and merge the PR, it really works.

@Dallas62
Copy link
Collaborator

Dallas62 commented Apr 9, 2020

@tankers746 Thanks for your implication, can you summarize which bugs is fixed ?
As I understand:

  • The Notification does appear in the notification center if the app is in Foreground
  • Custom Sounds are now fixed at notification level on Android

@HarshitMadhav @iamshadmirza @dabakovich
Did you test this PR?

Thanks!

@dabakovich
Copy link

dabakovich commented Apr 9, 2020

@Dallas62 @tankers746
Yes, tested it. The fix works for me. Hovewer I added an optional parameter showInForeground for Android platform so I am able now to choose whether to show this notification in foreground.
Fork here: dabakovich@06dcd7a

@Dallas62
Copy link
Collaborator

Dallas62 commented Apr 9, 2020

@dabakovich thank for this reply,
I saw this parameter in another PR if I remember well, but someone says that it should be handle in the JS side, using a .postLocalNotification.
If you do it in the JS side, I think the behavior is the same in iOS and Android. Has I remember, iOS does not show the notification if the App is in foreground, do you confirm?

@Dallas62
Copy link
Collaborator

Thanks for this PR!
I integrated it in master, but with some changes (missing description on channel and remove TMP variable).

@Dallas62 Dallas62 closed this in 7516032 Apr 20, 2020
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.

7 participants