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

feat(FCM): support Firebase Cloud Messaging plugin #1449

Merged
merged 7 commits into from
May 9, 2017

Conversation

klirix
Copy link
Contributor

@klirix klirix commented May 1, 2017

Added Firebase Cloud Messaging plugin.

Tested it out on Android, test on iOS is coming.

Event functions (onTokenChange, onNotification) repurposed to be observables.

Code feedback would be great, as it is my first plugin wrapper.

*
* @usage
* ```
* import { FCM } from 'ionic-native';
Copy link
Owner

Choose a reason for hiding this comment

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

Please add typescript to the markup ```typescript

* ```
* import { FCM } from 'ionic-native';
*
* constructor(private fcm: FCM) {
Copy link
Owner

Choose a reason for hiding this comment

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

The usage part is a bit to short. Can you describe the plugin a bit more? Orientate you by other plugins.
Don't put the plugin method call in to the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added additional examples in the usage section and in docs for each function

Copy link
Collaborator

@ihadeed ihadeed left a comment

Choose a reason for hiding this comment

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

Please remove any usage examples from above the method declarations. Keep all usage example in the main block.

*/
@Cordova({
observable: true,
successIndex: 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

this line is not needed

/**
* Get's device's current registration id
*
* ```typescript
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep all usage examples in the main block above the @Plugin decorator. Do not add any usage documentation above the method declaration.

* @returns {Promise<any>} Returns a promise resolving in result of subscribing to a topic
*/
@Cordova({
callbackOrder: 'reverse'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method should not have callbackOrder: 'reverse'. See https://github.com/fechanique/cordova-plugin-fcm/blob/master/www/FCMPlugin.js#L8

* @returns {Promise<any>} Returns a promise resolving in result of unsubscribing from a topic
*/
@Cordova({
callbackOrder: 'reverse'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method should not have callbackOrder: 'reverse'. See https://github.com/fechanique/cordova-plugin-fcm/blob/master/www/FCMPlugin.js#L12

@ihadeed ihadeed added this to the 3.6.0 milestone May 3, 2017
@ihadeed ihadeed merged commit 6cc6393 into danielsogl:master May 9, 2017
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.

None yet

3 participants