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

Allow listen to foreground or background notification separately #1524

Closed
wants to merge 1 commit into from

Conversation

lazywei
Copy link

@lazywei lazywei commented Jun 5, 2015

In many cases, the app need to respond to the notification in different ways.
Take chat app as an instance. The app need to jump to the certain chatroom when users opens the notification from background, while it shouldn't jump to anywhere when the chat app is in foreground.

So I split the notification type to foreground and background. In this way, the developers can listen to different type of notification accordingly.

Usage:

PushNotificationIOS.addEventListener("notificationForeground",
    this._handleNotificationForeground);
PushNotificationIOS.addEventListener("notificationBackground",
    this._handleNotificationBackground);

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 5, 2015
@lazywei lazywei force-pushed the remote-notification-type branch from 4699811 to a4c7b41 Compare June 10, 2015 03:47
@lazywei
Copy link
Author

lazywei commented Jun 10, 2015

@brentvatne I rebased this and made the test pass. Would you mind help me review this or assign a reviewer for this?
Thanks 😄

@brentvatne
Copy link
Collaborator

Hi @lazywei!

Some thoughts:

  • Is it more common to listen to both foreground and background or to listen to them separately? With this approach, we will need to add two listeners if we want to listen to both.
  • Could we use one event and pass in the event object whether it is foreground or background?

I don't have a lot of experience with this area so I'm curious what other smarter people like @ide think.

@ide
Copy link
Contributor

ide commented Jun 10, 2015

Probably makes more sense to put this logic in your app's JS -- when your JS gets the "notification" event, check whether the application is foregrounded or not.

I would also be surprised if this diff works... React Native does not work with backgrounded apps right now, and I think you need a different app delegate method to handle notifications while backgrounded.

@lazywei
Copy link
Author

lazywei commented Jun 11, 2015

Probably makes more sense to put this logic in your app's JS -- when your JS gets the "notification" event, check whether the application is foregrounded or not.

I agree with you. However, the thing is currently we don't have a proper way to check the app's foreground/background state (at least I'm not aware of).

I would also be surprised if this diff works... React Native does not work with backgrounded apps right now, and I think you need a different app delegate method to handle notifications while backgrounded.

Actually it works pretty well in my project (I'm using this PR in my app). It doesn't require RN work with backgrounded app I think. It's totally native solution -- only it emits different event for background / foreground app.
In a nutshell, the background notification will be triggered if we click the notification and jump to the app. On the other hand, the foreground notification will be triggered if the app is currently foregrounded.

@ide
Copy link
Contributor

ide commented Jun 11, 2015

@lazywei
Copy link
Author

lazywei commented Jun 12, 2015

oh, that's cool.
OK, it makes sense to do that in JS side then.
I'm going to close this PR.

Thanks.

@lazywei lazywei closed this Jun 12, 2015
@brentvatne
Copy link
Collaborator

Thanks for it anyways @lazywei, even if it didn't work out 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants