-
Notifications
You must be signed in to change notification settings - Fork 29
Update Google Pay #22
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
base: master
Are you sure you want to change the base?
Conversation
Minishlink
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
android/src/main/java/tech/bam/RNBraintreeDropIn/RNBraintreeDropInModule.java
Outdated
Show resolved
Hide resolved
| dependencies { | ||
| implementation 'com.braintreepayments.api:drop-in:4.+' | ||
| implementation 'com.facebook.react:react-native:+' | ||
| // Braintree dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better for users to make this a configuration step for using the library
Not everybody wants to have this in their code
See example of 3D Secure https://github.com/bamlab/react-native-braintree-payments-drop-in#3d-secure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| GooglePaymentRequest googlePaymentRequest = new GooglePaymentRequest() | ||
| .transactionInfo(TransactionInfo.newBuilder() | ||
| .setTotalPrice(String.valueOf(googlePayOptions.getDouble("amount"))) | ||
| .setTotalPriceStatus(WalletConstants.TOTAL_PRICE_STATUS_FINAL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that be configurable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain more detail please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be made in a follow-up PR, but apparently total price status can be changed to estimated or not known. This should be ideally an option in this bridge.
| if (googlePayOptions.hasKey("merchantName")) { | ||
| googlePaymentRequest.googleMerchantName(googlePayOptions.getString("merchantName")); | ||
| } | ||
| googlePaymentRequest.googleMerchantName("Simi"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That shouldn't be here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed line 77. Is that right?
| googlePaymentRequest.googleMerchantName("Simi"); | ||
| dropInRequest.googlePaymentRequest(googlePaymentRequest); | ||
| } else { | ||
| dropInRequest.disableGooglePayment(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line really necessary, and if yes does it work when you remove the gradle deps that you added earlier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is required to disable Google pay when developer doesn't pass "googlePay" parameter
|
I'll test this as soon as possible and come back to you, thanks for taking into account my review :) |
Minishlink
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the configuration and usage sections of Google Pay in the README please?
| implementation 'com.braintreepayments.api:drop-in:4.+' | ||
| implementation 'com.facebook.react:react-native:+' | ||
| // Braintree dependencies | ||
| implementation 'com.braintreepayments.api:braintree:3.7.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After testing, you don't need to add this dependancy (it might be a sub deps of the drop-in)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you show me your testing without these dependencies? I still haven't figured out the way to do it
| implementation 'com.facebook.react:react-native:+' | ||
| // Braintree dependencies | ||
| implementation 'com.braintreepayments.api:braintree:3.7.0' | ||
| implementation 'com.braintreepayments.api:google-payment:3.2.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After testing, you don't need to add this dependancy (it might be a sub deps of the drop-in)
| implementation 'com.braintreepayments.api:google-payment:3.2.0' | ||
|
|
||
| // Google dependencies | ||
| implementation 'com.google.android.gms:play-services-wallet:16.0.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't force the version
See example
| @@ -0,0 +1,9 @@ | |||
| arguments= | |||
| auto.sync=false | |||
| gradle.user.home= | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant the whole file, it should be ignored by the gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just remove this file and .settings already in gitignore

Hi, please review my PR. We are working with Braintree and your library is great with credit card and Paypal option. However, we need Google Pay works as well.