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 network plugin for issue #89 #90

Merged
merged 2 commits into from
Mar 28, 2016
Merged

add network plugin for issue #89 #90

merged 2 commits into from
Mar 28, 2016

Conversation

keithdmoore
Copy link
Contributor

add cordova-network-information plugin

@keithdmoore
Copy link
Contributor Author

@mlynch @tlancina I thought I had committer access to this repo. Maybe I only have committer access to the ngCordova repo. Would you or Tim mind giving me committer access to ionic-native?

@ihadeed
Copy link
Collaborator

ihadeed commented Mar 28, 2016

Thanks for the PR @keithdmoore

  1. No need to import the Connection interface into index.ts
  2. The plugin hasn't been added yet. I'm working on it in the add-network-info branch here: https://github.com/driftyco/ionic-native/blob/add-network-info/src/plugins/networkinformation.ts

@mlynch
Copy link
Collaborator

mlynch commented Mar 28, 2016

@keithdmoore done!

@ihadeed
Copy link
Collaborator

ihadeed commented Mar 28, 2016

Never mind my previous comment. Just saw your recent commit @keithdmoore

@keithdmoore
Copy link
Contributor Author

@mlynch Thanks!
@ihadeed Sorry, I must have missed the fact that there was already an issue opened for this. I implemented it fully. Unless you disagree with my implementation, can we pull it in?

@ihadeed
Copy link
Collaborator

ihadeed commented Mar 28, 2016

@keithdmoore

Shouldn't static get connection() be static get type() to match the original plugin?

@keithdmoore
Copy link
Contributor Author

@ihadeed I can change that if you want. I went back and forth on that. I did deviate a little with the event handlers. onConnect and onDisconnect seemed to make more sense to me. onOnline and onOffline sound and look weird.

@ihadeed
Copy link
Collaborator

ihadeed commented Mar 28, 2016

@keithdmoore Yeah onOnline does sound weird.

The reason I said change it to type because AFAIK the @CordovaProperty overrides the return value you have there by using the property name.
Reference: https://github.com/driftyco/ionic-native/blob/master/src/plugins/plugin.ts#L240-L259

@keithdmoore
Copy link
Contributor Author

@ihadeed I am having trouble getting type to work. Were you able to get type to work? My committed code works with the usage examples in an app I am working on.

@ihadeed
Copy link
Collaborator

ihadeed commented Mar 28, 2016

@keithdmoore Ahh. type could be a reserved word.

If the code you wrote works then I think that's fine. I just thought the @CordovaProperty would overwrite it. I guess it's just there to check if the plugin and Cordova exist.

@ihadeed ihadeed merged commit f2e274f into danielsogl:master Mar 28, 2016
@keithdmoore
Copy link
Contributor Author

@ihadeed Fair enough. I just realized I left a comment that should be removed. Ugggg.

@ihadeed
Copy link
Collaborator

ihadeed commented Mar 28, 2016

@keithdmoore this one ed14d87 ?

@keithdmoore
Copy link
Contributor Author

@mlynch Looks like you gave the ngCordova team (which I am on) read access to ionic-native. Can we get admin access?

@ihadeed
Copy link
Collaborator

ihadeed commented Mar 28, 2016

@keithdmoore @mlynch your input is appreciated here #86

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