Vizir / react-native-paypal

Native Paypal payment screen for React Native
MIT License
51 stars 15 forks source link

iOS #1

Open amiuhle opened 8 years ago

amiuhle commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

MattFoley commented 8 years ago

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

MattFoley commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

MattFoley commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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. https://github.com/MattFoley/react-native-paypal/pull/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 commented 8 years ago

@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!