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

iOS #1

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

iOS #1

wants to merge 37 commits into from

Conversation

amiuhle
Copy link
Contributor

@amiuhle amiuhle commented Jan 13, 2016

I started working on an iOS implementation. So far I've only merged in MattFoley/react-native-paypal, so it's still a work in progress.

I just wanted to let people know to avoid duplicate work on this.

@cartolari
Copy link
Contributor

Thanks for the PR but there are some issues with it:

  1. As you already pointed out in the MattFoley/react-native-paypal repo there is no license to the code.
  2. The PR is too big
  3. Does not maintain JavaScript compatibility, which should, so users of this plugin can use the same JavaScript interface regardless of platform
    If @MattFoley does not change the license in the repo we won't be able to merge this PR, also I recommend you rework it and put all iOS specific stuff in an "ios" folder in the root of the repo

@amiuhle
Copy link
Contributor Author

amiuhle commented Jan 13, 2016

I'm aware that there's no JavaScript compatibility right now, I'm working on that. Like I said, it's a work in progress, and I don't expect you to merge before it's finished.

As for the licensing issues, let's just hope @MattFoley will add one.

Or are you guys already working on an iOS implementation yourselves?

@cartolari
Copy link
Contributor

We'll need one by the next month for a project, so if we do not find a suitable implementation till there we'll build one.
And like we already said, there are licensing issues with the current implementation, so the alternatives we have if a license is not added to the project is:

  1. You could build one version working on top of the current structure, without forking @MattFoley
  2. We release our version probably next month.

@MattFoley
Copy link

I'd be happy to add a license. Sorry about that, usually my code is MIT licensed.

@MattFoley
Copy link

There ya go. Beerware this time, if that suits you gentlemen.

@MattFoley
Copy link

I'd be very happy if you guys went ahead and used this. I won't be able to implement Android support and I'm very much a javascript novice. Let me know if you need anything else.

@amiuhle
Copy link
Contributor Author

amiuhle commented Jan 14, 2016

@MattFoley Thanks for the license, if I ever come to Boston I'll buy you that beer ;)

I moved the license to ios/LICENSE.md for now because I'm not sure how to handle MIT and Beerware in the same license file...

@cartolari I've got it working. I'll do some more (manual) testing tomorrow, but it seems like single payments work just like in Android, with the same API for the user.

Internally, the API is different though. I'm not an iOS developer and I'm not familiar with the PayPal SDK and the differences between the iOS and the Android SDK. You can either merge this PR and then refactor, or at least take a look at my implementation and then do it from scratch.

I struggled with integrating the PayPal SDK into the library and linking it all together, so if you do it from scratch, you might want to have a look at the updated README about the installation instructions for the end user and at my commits dbcab7c and e73c131. Basically I followed the manual instructions from PayPal, doing steps 1-4 for the library project, and steps 4-5 for the app (note the duplicate step 4).

I also played around with CocoaPods but failed. In the end, I think the way I did it is quite nice for the users of the library. But like I said, I'm not an iOS developer, so maybe there are easier / better ways.

For development, the PayPal SDK is in the repository as a submodule.

@cartolari
Copy link
Contributor

Thanks @amiuhle,
I'll give a look at your commits and the README.

@MattFoley
Copy link

If you guys are still working on this, you may want to eyeball some updates I made to my repository recently. A few issues were found when used with the latest version of React Native and they're all fixed and in the master branch over there now.

@MattFoley
Copy link

MattFoley commented May 23, 2016

Looks like iOS support was never merged here? Should I go ahead and bring in this repository into mine, so that we have one unified iOS and Android paypal wrapper?

@miracle2k
Copy link

I'm also interested in working on a unified wrapper. My intention was to start with this pull request and unify the two APIs. Is there a better starting point?

@chagasaway
Copy link

Hey @MattFoley and @miracle2k,

Sorry for the delay... I am without time to spend in the iOS support by now (since the product that we were using this package is no longer using PayPal).

I am adding you both as collaborators here, but feel free to make the universal wrapper on your own repositories.

Best regards,

@MattFoley
Copy link

If that's the case, I'd love to move an Android wrapper into my repository.

We use the iOS version at Skillz, and will need an Android version in the coming months. I can't imagine we would stop using it, so it will have a stable maintainer for a long time to come.

On May 30, 2016, at 9:17 AM, Fellipe Chagas notifications@github.com wrote:

Hey @MattFoley and @miracle2k,

Sorry for the delay... I am without time to spend in the iOS support by now (since the product that we were using this package is no longer using PayPal).

I am adding you both as collaborators here, but feel free to make the universal wrapper on your own repositories.

Best regards,


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@amiuhle
Copy link
Contributor Author

amiuhle commented May 31, 2016

@MattFoley I think It'd be easier to just merge my PR into this repository, but either way, if you need any help, let me know.

@MattFoley
Copy link

Sorry about the long wait on getting this merged together. I've opened a copy of your PR against our repository, which we'll be using in production, so will be stable and maintained. MattFoley/react-native-paypal#11

I did make a few minor adjustments:
• I've moved the index.js file a bit closer to ES6 syntax, but not quite finished.
• I've brought back in the iOS sample app.

Will this suit your needs going forward @amiuhle? I'm going to work with another engineer on our team to review the code, but it should be merged in tomorrow.

@amiuhle
Copy link
Contributor Author

amiuhle commented Aug 15, 2016

@MattFoley I'm guessing so, but unfortunately I don't have time to check right now. Just go ahead and merge. If there are any necessary adjustments, I'll create a PR when I'm working on the project the next time.

Thanks for the 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.

5 participants