using Google Pay button API in demo app#1606
Conversation
noguier
left a comment
There was a problem hiding this comment.
Hello, can you please add a CHANGELOG entry to record new playServicesWallet = "19.5.0" dependency
| contentAlignment = Alignment.Center | ||
| ) { | ||
| Column(horizontalAlignment = Alignment.CenterHorizontally) { | ||
| if (isReadyToPay && allowedPaymentMethods.isNotEmpty()) { |
There was a problem hiding this comment.
should we give user a feedback in case allowedPaymentMethods are empty here?
| "USD", | ||
| "1.00", | ||
| GooglePayTotalPriceStatus.TOTAL_PRICE_STATUS_FINAL, | ||
| isPayPalEnabled = false |
There was a problem hiding this comment.
if I set isPayPalEnabled to true nothing renders in this fragment
There was a problem hiding this comment.
Yes. The dynamic Google Pay button doesn't work if there is an "type": "PAYPAL" object in the allowedPaymentMethods array.
There was a problem hiding this comment.
Interesting. Do you know why that is?
There was a problem hiding this comment.
The dynamic button shows the the card art and last4 digits (if the user had a card on file) and therefore primarily works with "type": "CARD".
c6f6829 to
5fe2d0f
Compare
Done. Sorry about that. |
noguier
left a comment
There was a problem hiding this comment.
Thank you for addressing my comments!
|
@noguier anything more needed from my side or is this ready to merge? 🙂 |
|
@dmengelt we are just waiting on one more review from my team, thank you for your patience! |
saralvasquez
left a comment
There was a problem hiding this comment.
This button looks super cool! I also appreciate the labor to move over to compose. I just have a couple of comments that could use a look. Thanks!
| * BraintreeCore | ||
| * Allow fetching of `Configuration` to query supported integrations and card types (fixes #1302) | ||
| * Google Pay | ||
| * Update Google Pay dependency (play-services-wallet) to version 19.5.0 |
There was a problem hiding this comment.
Can you please move this under an unreleased header at the top of this file? We also try to keep spacing to 4 space tabs, could you try to match that for this entry as well?
There was a problem hiding this comment.
I actually had this under the unreleased header: 5fe2d0f#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR7
Updated again 👍
| "USD", | ||
| "1.00", | ||
| GooglePayTotalPriceStatus.TOTAL_PRICE_STATUS_FINAL, | ||
| isPayPalEnabled = false |
There was a problem hiding this comment.
Interesting. Do you know why that is?
| val request = GooglePayRequest( | ||
| "USD", | ||
| "1.00", | ||
| GooglePayTotalPriceStatus.TOTAL_PRICE_STATUS_FINAL, | ||
| isPayPalEnabled = false | ||
| ) | ||
| googlePayClient.createPaymentAuthRequest(request) { authRequest -> | ||
| if (authRequest is GooglePayPaymentAuthRequest.ReadyToLaunch) { | ||
| val json = | ||
| JSONObject(authRequest.requestParams.paymentDataRequest.toJson()) | ||
| allowedPaymentMethods = | ||
| json.getJSONArray("allowedPaymentMethods").toString() | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct me if I'm reading this wrong, but this whole series of operations looks like a throw away just to gather the value of allowedPaymentMethods. Can you find a way to gather this information without making unnecessary calls? On top of this being slower, createPaymentAuthRequest triggers analytics that would just be noise
| cardinal = { group = "org.jfrog.cardinalcommerce.gradle", name = "cardinalmobilesdk", version.ref = "cardinal" } | ||
| paypal-messages = { module = "com.paypal.messages:paypal-messages", version.ref = "paypalMessages" } | ||
| play-services-wallet = { group = "com.google.android.gms", name = "play-services-wallet", version.ref = "playServices" } | ||
| play-services-wallet = { group = "com.google.android.gms", name = "play-services-wallet", version.ref = "playServicesWallet" } |
There was a problem hiding this comment.
If this rename isn't strictly necessary could you please leave this as is?
There was a problem hiding this comment.
I can revert this but playServices is misleading IMO 🙂 WDYT?
There was a problem hiding this comment.
Yeah I agree it's not the most intuitive naming. I generally try to keep unnecessary changes to a minimum since it's critical to keep current integrations functioning as expected but I was too hasty here. Renaming something in this file should be 100% safe since it's only internally accessible. Feel free to leave it
992fe17 to
e1642cb
Compare
|
@saralvasquez let me know if any more changes are needed. |
e1642cb to
68d3ded
Compare
|
Fixed the Android Lint errors |
Signed-off-by: Sara Vasquez <98496950+saralvasquez@users.noreply.github.com>
|
@saralvasquez any clue why the dependency-review fails? |
Summary of changes
play-services-walletdependency to19.5.0screen-20260527-172756-1779895658031.mp4
AI Usage
Which AI Agent Was Used?
How was AI used?
Estimated AI Code Contribution
Checklist
Authors