Implement platform pay payment method capturing for web #1565
Conversation
cornwe19
commented
Dec 20, 2023
cornwe19
commented
- Addresses Google pay and Apple pay with Web #1122
- Includes changes for detecting platform pay availability on web as well. (see Add support for checking platform pay availability on web #1478)
|
@jonasbark @jamesblasco @remonh87 this supersedes #1478 |
|
@jonasbark @jamesblasco @remonh87 looking for feedback on this PR. It addresses a sizeable feature gap that exists on web. |
There was a problem hiding this comment.
Many thanks for this PR. I did review it and in general it looks good. However I am not convinced about adding the web paymentrequest options to isPlatformPaysupported interface. @jonasbark what are your thoughts about it?
| @@ -99,6 +99,7 @@ abstract class StripePlatform extends PlatformInterface { | |||
| /// Check if either google pay or apple pay is supported on device. | |||
| Future<bool> isPlatformPaySupported({ | |||
| IsGooglePaySupportedParams? params, | |||
| PlatformPayWebPaymentRequestCreateOptions? paymentRequestOptions, | |||
There was a problem hiding this comment.
I do not like the fact we need to inject a web specific method into the platform interface this should be independent. Is this really required for just checking if platformpay is supported?
There was a problem hiding this comment.
Honestly, I agree, it'd be preferable to hide the web implementation details here, but on web there is no implicit payment method (e.g. Apple Pay would be the only thing available in iOS apps), so without the ability to minimally filter which payment methods you want to support on web, we'd be limiting developers using the Flutter Stripe package on web pretty severely.
Note that the web options, much like the Google Pay options already passed to this method, are optional and come with a reasonable default. This means no breaking change and if they aren't supplied on web, will simply check for the availability of any wallet pay solution including Stripe's own Link Pay.
There was a problem hiding this comment.
@remonh87 thoughts on my explanation above? Do you have suggestions for a different approach here given the constraints?
There was a problem hiding this comment.
no looks to me. I noticed some conflicts in this branch but we are good to go. @jonasbark do you want to have a look at it as well?
There was a problem hiding this comment.
Excellent. Thanks for your input. I'll get the conflicts cleared up shortly
cornwe19
force-pushed
the
feature/1122-wallet-pay-web-payment-method
branch
from
5a1a1b8 to
5c92c14
Compare
|
@remonh87 I've resolved conflicts and reformatted code based on your suggestions. Let me know if there are any other outstanding items on this one. Excited to get this merged through c.c. @jonasbark |
|
Thanks for the work. I am running the CI now and give Jonas till tomorrow to have a look at it else I will merge it. |
|
@remonh87 Thanks! I'm seeing some failures like |
|
@remonh87 thoughts on getting this merged today? |
|
@remonh87 any remaining items blocking this pr? |
* Partially addresses flutter-stripe#1122
* Call to show is inside async block anyway, so logistically, nothing really changes here, but the intent of the code reads better with these declared before 'show' *
cornwe19
force-pushed
the
feature/1122-wallet-pay-web-payment-method
branch
from
8f5327f to
ab6cab3
Compare
|
@remonh87 Just synced this PR with latest main. I suspect checks will need to be rerun and will probably fail in similar ways to how they did on Jan, 24th. Please let me know if there's any action needed here. Hoping to get this PR merged soon. Thanks! |
|
Excited to see the PR to be merged |
|
@remonh87 FYI I just realized the way I had implemented payment request cancellation wasn't aligned with how iOS and Android implement it. Added a commit to update that (it now uses a Looking forward to getting this merged. |
|
Thanks for the great work! I am wondering when this feature will be released. @remonh87 |
