Update Express Payments Loading UI #4228
Conversation
|
Size Change: +2.8 kB (0%) Total Size: 991 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
EDIT: As of the latest rebase on May 27, I can't reproduce this issue anymore.
I'm seeing an issue with Chrome Pay on this branch that I'm not seeing on trunk. I have doubled checked this by first finding the issue on this branch. Then switching to trunk, doing a clean install I wasn't able to reproduce the issue. Then switching back to this branch doing a clean install the issue resurfaced again.
I've sent you a DM with a screen recording that I won't share here due to it containing private data. But I'll describe the issue in case we need someone else to see if they can reproduce it.
What happened
In Chrome, I click the Buy Now button. This triggers the Chrome modal for express payment. The Pay button is greyed out. I select a Delivery Address. This leads to a spinner showing in the Chrome modal for 60 seconds before the modal closes itself / crashes. I click Buy Now again, the modal reopens. The Pay button is still grey. I select a Delivery Address, this time the selection is instant. The Pay button turns blue and I can check out successfully. Now the payment is registered in the Stripe backend.
What should've happened
The modal shouldn't have closed itself. Instead, the selection should've updated and the Pay button should've become active.
mikejolley
force-pushed
the
update/express-payments-loading-ui
branch
from
226cd43 to
135ec39
Compare
assets/js/blocks/cart-checkout/payment-methods/express-payment-methods.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Testing isn't going too well for me in Edge using the Browser pay modal:
When there is a shopper cached in the browser instance (i.e. address details etc will get filled in on loading the checkout). I get an error when setting the shipping address in the browser pay modal. I've verified the same conditions do not produce an error in trunk. I highly suspect this is due to how we're passing through the paymentMethodInterface data. Under the right conditions the data is stale.
The other issue is that this isn't adding a loading transition or preventing the express payment button from being clicked again when a card requiring 3DS confirmation is used:
Payment method content could potentially become a bottlen...Find a way to Memorize Express Payment Method Content Payment method content could potentially become a bottleneck if lots of logic is ran in the content component. ItCurrently re-renders excessively but is not easy to useMemo because paymentMethodInterface could become stale.paymentMethodInterface itself also updates on most renders. https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/5ba2c97143fc34f1f0237096261b8255b9dc7133/assets/js/blocks/cart-checkout/payment-methods/express-payment-methods.js#L61-L76🚀 This comment was generated by the automations bot based on a
|
|
I removed the The other issue, 3D Secure in latest Stripe, is not resolved. I added loading state for I've moved this back in progress and removed my assignment. Ill pick it up again next week if no-one gets to it before I'm back from AFK. |
This prevents the potential of having an undefined value returned.
These do not need to be used as dependencies.
This prevents children being rerendered and losing state. Loading styles are applied instead using a classname, but leaving the divs in place.
mikejolley
force-pushed
the
update/express-payments-loading-ui
branch
from
28a2809 to
80203c9
Compare
There was a problem hiding this comment.
Wow, there's some really nice refactoring in here (really like moving logic out of the payment method context data provider into custom hooks...makes the code easier to read) 👏🏻
Summary of feedback sprinkled through inline comments:
- Some concerns around change in html structure (as surfaced in test snapshots as well) and how that may impact existing styling. Likely not a problem but still had questions.
- A question around loading interactivity restrictions.
- A requested change around unnecessary use of
useShallowEqualfor boolean values. - Maybe add some testing notes (or even better, automated tests) to cover change to
dispatchCheckoutEvent. - Might be some opportunity to clean up the overlap between
isDoingExpressPaymentandexpressPaymentMethodActive(but could be done in a followup).
Besides the above, regarding onExpressPaymentError:
- I like the implementation and think it works well.
- I'm not sure on the name
onExpressPaymentError. I think it shares similarity to the naming structure for other event subscribers but itself isn't a subscriber function. - I would like to see
setExpressPaymentErroras the name of the new method and you move in the setting of notices into the new method. I don't think we need to have two separate behaviours here.
I did very basic testing of Stripe (with your branch for the gateway) and as far as I can tell it works great! However, I'm really leaning on your own testing for complete validation of payment processing flow.
I do plan on trying to setup WooCommerce Payments and test with it too before giving the ✅ .
assets/js/base/context/hooks/payment-methods/use-payment-methods.js
Outdated
Show resolved
Hide resolved
| doAction( | ||
| `experimental__woocommerce_blocks-checkout-${ eventName }`, | ||
| { | ||
| ...eventParams, | ||
| storeCart, | ||
| cartParam, | ||
| } |
There was a problem hiding this comment.
This is a significant change and testing notes don't cover it (and afaict there are no automated tests covering this - although I could be mistaken). Should there be some mention of coverage in testing notes and/or add some automated tests around behaviour?
I do see how this improves the memoization of dispatchCheckoutEvent given the high volatility of store cart value. On the surface this seems like there shouldn't be any unintended side-effects of the change given cartParam should always reflect the current store state when dispatchCheckoutEvent is invoked.
There was a problem hiding this comment.
I know why I did this (it's used as a dependency in various places and I didn't want to trigger the effects when the cart changed).
The change was to store the cart as a ref so the callback function can use the ref instead of using cart as a dependency. I did a quick test to make sure I understand refs and callbacks so it will have the latest cart object available (https://codesandbox.io/s/clever-brook-i8fuy?file=/src/app.js).
I know tests would be good, but I'm still not confident writing tests from scratch and I know it will take me the rest of the week to fathom.
There was a problem hiding this comment.
I've restored the storeCart parameter name here; that was an oversight on my part. It's still using the ref.
There was a problem hiding this comment.
I know tests would be good, but I'm still not confident writing tests from scratch and I know it will take me the rest of the week to fathom.
I'm okay with it not blocking the PR, but I do think it'd be worthwhile doing this as a followup. We need more confidence when changing things and automated tests are a good way to get that.
assets/js/base/context/providers/cart-checkout/payment-methods/constants.ts
Show resolved
Hide resolved
assets/js/base/context/providers/cart-checkout/payment-methods/payment-method-data-context.tsx
Show resolved
Hide resolved
assets/js/base/context/providers/cart-checkout/payment-methods/reducer.ts
Show resolved
Hide resolved
assets/js/blocks/cart-checkout/checkout/sidebar/test/__snapshots__/index.js.snap
Show resolved
Hide resolved
assets/js/blocks/cart-checkout/payment-methods/express-payment-methods.js
Outdated
Show resolved
Hide resolved
assets/js/blocks/cart-checkout/payment-methods/express-payment-methods.js
Show resolved
Hide resolved
Find a way to block buttons/form components when LoadingM...Find a way to block buttons/form components when LoadingMask isLoading https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/b8ba959ee00eb6f58c424aca063f6d682d9dd242/assets/js/base/components/loading-mask/index.js#L14-L17🚀 This comment was generated by the automations bot based on a
|
There was a problem hiding this comment.
Looks great Mike! I tested this with WooCommerce Payments and things check out there.
The only remaining issue I noticed is that when the express payment method is processing after completing the 3DS verification, the "Place Order" button is still clickable (and it's possible to edit fields in the checkout. The only thing blocked is the express payment method area. We probably should be blocking the entire checkout?
Up to you whether you handle this in a followup or not but I do think it'll need addressed. Pre-approving pending your decision about the last point.
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
There was a problem hiding this comment.
I tested the latest changes with WooCommerce Payments, Stripe Extension, built in blocks stripe integration and also with the Stripe Extension onClose fix branch. I tested GooglePay (via chrome) and Browser Payment Request (via Edge). As far as I could tell most behaviour works okay.
However, in my testing I am seeing some issues with switching shipping address in the express payment method and that rates being updated. While I'm reproducing this in trunk, it does give me pause to rely on just my testing for this. My setup is:
- display taxes inclusive in Cart and Checkout
- Different shipping zones with different rates based on address and postal code.
- When I trigger GooglePay, I'll either won't get all the expected rate options initially (and a different rate selected) or I will get the expected rate options but switching the address in Google Pay does not update the rates as expected.
- Rates in the Browser Pay modal update just fine.
- I'm reproducing this for both built-in Stripe integration in blocks AND the Stripe extension integration. Further, with the Stripe extension I'm also reproducing on the shortcode flows.
So it'd be good to have someone else test and:
- see if you can reproduce what I experienced above in this branch.
- If yes to the above, see if you can reproduce in trunk too. If yes, then not a blocker to merging this PR.
- If you can't reproduce my issue at all, then let me know, we'll have to sync up on what might be in my configuration that's triggering it (or perhaps I have something borked on my db).
I'm pre-approving because other than the above issue, things test well. I do think additional testing to verify whether the above issue I've reported is valid or not though.
|
@nerrad I seem to only have issues when using this branch and Stripe Trunk. I see matching behaviour between trunk and this branch when using my Stripe onClose handling Branch. Does this line up with what you are seeing? In the broken setup, there is no initial shipping address. For the working setup, I get an initial rate, and can rate switch, and paying has the correct amount, but I do see the wrong amount on the checkout in the background. |
|
@nerrad I got it working by adding Check your side? |
|
Just leaving a comment here to note that Mike and I did some synchronous testing together to validate and clear the issues I reported. All was good. |
This PR makes some improvements to the Express Payments area and related components, notably:
useShallowEqualnow returns a default value instead of undefined. Updated typescript hints to match.Fixes #4159
How to test the changes in this Pull Request:
To test this properly you'll need Stripe setup locally in sandbox mode. You can test the express payments with Chrome/Edge Pay, or Apple Pay in Safari.
Changelog