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

Add new Firebase phone verification methods #2157

Merged
merged 3 commits into from
Dec 2, 2017

Conversation

nikmartin
Copy link
Contributor

No description provided.

Copy link
Contributor

@janpio janpio left a comment

Choose a reason for hiding this comment

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

Strange formatting changes, but actual change looks good. 👍

@@ -30,89 +30,108 @@ import { Observable } from 'rxjs/Observable';
plugin: 'cordova-plugin-firebase',
pluginRef: 'FirebasePlugin',
repo: 'https://github.com/arnesson/cordova-plugin-firebase',
platforms: ['Android', 'iOS']
platforms: ['Android', 'iOS'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the added , here?


/**
* Get notified when a token is refreshed
* @return {Observable<any>}
*/
@Cordova({
observable: true
observable: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the added , here?


/**
* Get notified when the user opens a notification
* @return {Observable<any>}
*/
@Cordova({
observable: true
observable: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the added , here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My apologies, TSLint has a config option on by default that adds a trailing comma, so when you need to extend an object or array, you can just start typing. I've grown used to it because I'm lazy :)


/**
* Grant permission to receive push notifications
* @return {Promise<any>}
*/
@Cordova({
platforms: ['iOS']
platforms: ['iOS'],
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

@nikmartin
Copy link
Contributor Author

Also, there has been a bit of code rot in the underlying cordova plugin, I'm working with the repo owner to try to get him to re-publish to npm. He forgot to bump the package version when merging the PR this PR is related to, so the package on npm is out of date

@ihadeed ihadeed merged commit 86181af into danielsogl:master Dec 2, 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.

3 participants