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

added popInitialNotification function #19

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

Conversation

jawadrehman
Copy link

No description provided.

@jawadrehman
Copy link
Author

This function allows you to access the "payload" sent with your notification.

@oney
Copy link
Owner

oney commented Dec 26, 2015

Thanks!

One question.
Why don't you put popInitialNotification in public Map<String, Object> getConstants() rather than here
I think putting it in getConstants will be more like official react native implementation

@jawadrehman
Copy link
Author

@oney I didnt realize that could be done. I think that will be a better approach since then we dont need to use an event listener. Also I was thinking that we need to clear that notification after it has been "popped".

@oney
Copy link
Owner

oney commented Jan 18, 2016

Currently, this module uses https://github.com/Neson/react-native-system-notification to create notification in 0.1.7. You can get payload of clicked notification in DeviceEventEmitter.addListener('sysNotificationClick' listener. Please see whether it meets your requirements.
I still more like use popInitialNotification way to get payload, but I prefer to handle creating notification in react-native-system-notification rather than react-native-gcm-android.

@jawadrehman
Copy link
Author

DeviceEventEmitter.addListener('sysNotificationClick' doesnt work when the app is not already open.

@RGBz
Copy link

RGBz commented Jan 22, 2016

Yeah DeviceEventEmitter.addListener('sysNotificationClick'... only works when the app is open. popInitialNotification would be ideal.

@oney
Copy link
Owner

oney commented Jan 22, 2016

@RGBz absolutely agree with you. I will remind the author of react-native-system-notification this issue, and wait him to fix it.

@RGBz
Copy link

RGBz commented Jan 22, 2016

Thinking about it a bit more, a callback-based approach like this implementation of popInitialNotification or DeviceEventEmitter.addListener('sysNotificationClick'... is not what you'd want to handle opening the app via notification because it means the app will:

  1. Open up and start rendering a screen (like an apps home screen)
  2. Process the DeviceEventEmitter.addListener('sysNotificationClick'... callback
  3. Render the screen you'd want to show for the notification

The PushNotificationIOS.popInitialNotification API returns its value inline which instead of using a callback. This would allow for you to open the app right up to the screen you'd want on the first render pass.

@oney What are your thoughts? If you agree I can look into making a popInitialNotification that returns its value inline as well.

@oney
Copy link
Owner

oney commented Jan 22, 2016

My opinion is same as you, directly get payload from popInitialNotification like PushNotificationIOS.popInitialNotification, not from or on a callback. I have mentioned here.
Because the notification is created from react-native-system-notification module, the better way of this implementation is making PR in that module instead of here.

@RGBz
Copy link

RGBz commented Jan 22, 2016

I agree, I'll hop over there. Thanks!

@jawadrehman
Copy link
Author

I agree a more inline approach would have been suitable. Wasn't sure how to make that work.

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