Final PR from aspect-apps #8
Conversation
0xamogh
commented
Sep 6, 2020
0xamogh
commented
- Setup Intents for iOS and Android
- Updated methods to support payments from saved Cards
…rmPaymentWithCardParams
Update confirmSetupIntent return object
last4 added
…onfarrell docs: add lukebrandonfarrell as a contributor
docs: add ChesterSim as a contributor
docs: add amogh-jrules as a contributor
|
Looks solid! Code reviewing it now! |
| @@ -49,6 +49,7 @@ android { | |||
| targetSdkVersion safeExtGet('targetSdkVersion', DEFAULT_TARGET_SDK_VERSION) | |||
| versionCode 1 | |||
| versionName "1.0" | |||
| multiDexEnabled true | |||
There was a problem hiding this comment.
is this just a precaution or did you face any issues?
| @@ -0,0 +1,32 @@ | |||
| export interface InitParams { | |||
There was a problem hiding this comment.
build artifacts (build/*) should be gitignored
| @@ -0,0 +1,6525 @@ | |||
| { | |||
There was a problem hiding this comment.
In RN community yarn is more popular package manager therefore there already is yarn.lock file. I think we should avoid having two lock files because it will be hard to have them in sync.
.gitignore
Outdated
| # BUCK | ||
| buck-out/ | ||
| \.buckd/ | ||
| *.keystore |
There was a problem hiding this comment.
What's the intention behind this removal?
There was a problem hiding this comment.
Not sure, one of our devs may have removed it @amogh-jrules @ChesterSim
|
Hey @viktorasl, thanks for the in-depth review :) I think @amogh-jrules or @ChesterSim will be able to address some of these issues. I believe we did run into issues with Multidex |
| async confirmSetup(clientSecret: string, cardParams: CardParams): Promise<SetupIntentResult>{ | ||
| const nativeSetupIntentResult = await StripePayments.confirmSetup(clientSecret, cardParams); | ||
| const cardNumber = cardParams.number; | ||
| const cardType = creditCardType(cardNumber); |
There was a problem hiding this comment.
I'm wondering if retrieval of card type (brand) should be part of this library. There are a few reasons:
- Stripe already has method to get card type, of course it it's a separate method, but that looks like a valid separation of concerns
- I'd like to avoid dependencies (except obviously stripe)
Thinking further about use cases - after confirmation list of cards will still be fetched from back-end (to get full list of cards) therefore it's not necessary to have this IMHO
There was a problem hiding this comment.
@viktorasl we could move the card type retrieval to use the native stripe method as part of this method or a separate method.
There was a problem hiding this comment.
separate method would be more appropriate I think
|
Apologies for the late response, the gitignore changes have been made, we were having some trouble installing it locally |
…vieira docs: add rodriigovieira as a contributor
|
how can I use this branch in my project? I did I really need some of the features from this branch, I even tried manually creating the native modules and using it like:
NativeModules.StripePayments.init('publishable_key')
...
NativeModules.StripePayments.confirmSetup('client_secret_from_backend', cardParams)but got the same error 😕 . Cleaned build folder, reinstalled Pods, restarted the metro server and ran again, but same error. I must be missing something but not sure what. |
