Add new hooks related to payment methods and checkout and remove obsolete. #1929
Conversation
- Also restructures hooks directory to make things a bit more organized - Add useStoreOrder placeholder (followed up in future pull) - Add useBillingData hook.
- add useCheckoutSubmit which exposes checkout submit button interface - add useCheckoutRedirectUrl which exposes checkout redirect url interface
nerrad
added
focus: blocks
There was a problem hiding this comment.
Tested this and found no issues. Here's what I tested:
- Existing cart / checkout blocks render in front end and function correctly.
- In editor can add new cart / checkout blocks with no errors, publish, they function correctly in front end.
- Tested various options of the blocks:
- Cart totals / quantity / removing items, coupons, show/hide shipping calculator
- Checkout address entry, show/hide privacy policy link
I've added comments here and there - mostly around naming/documentation (none of this is blocking). I'm still getting my head around exactly what things like billing_data, checkoutRedirectUrl, paymentMethodInterface are - it would be great to document how all these pieces fit together at a very high level, using terms that relate directly to the Woo-merchant experience/UI (where possible). (Maybe best to tackle that later, perhaps in a cooldown.)
The only essential follow up from this review is to ensure all the @todos follow the rules of todos, i.e. there is an issue and it's linked in the code.
| * callbacks that will fire after | ||
| * selecting a shipping rate | ||
| * successfully. | ||
| * @property {function()} onShippingRateSelectFail Used to subscribe callbacks |
There was a problem hiding this comment.
Could probably factor out the "used to subscribe callbacks firing when" here into a common doc that applies to all :)
There was a problem hiding this comment.
Sorry, not sure I follow what you mean here? Each of these observer registration functions are for something different, so while they all are for subscribing observers to an event (the common bit of the doc), they are for specific events.
The doc blocks are useful for getting IDE tips when hovering over an object given this named type - moving it somewhere else and referencing it would lose that description for a property.
It's entirely possible I'm missing your intent by this comment though and you had in mind something that doesn't affect the above?
| /** | ||
| * @typedef CheckoutStatusProps | ||
| * | ||
| * @property {boolean} isCalculating If true then totals are being calculated in |
There was a problem hiding this comment.
Can some of these status concepts be defined in one place, since they are similar? Examples: complete, calculating, processing
There was a problem hiding this comment.
My response is the same as here
| */ | ||
|
|
||
| /** | ||
| * @typedef PaymentStatusProps |
There was a problem hiding this comment.
Would be great to have a one-liner for each of these types clearly defining the purpose. Sometimes that's more useful than the individual fields. I realise this is not an easy task, we can iterate on this as follow up :)
There was a problem hiding this comment.
Sure can do in a follow-up where more explanation is needed.
| isActive: true, | ||
| checkoutData, | ||
| paymentEvents, | ||
| activePaymentMethod: paymentMethod.id, |
There was a problem hiding this comment.
I've seen that payment methods sometimes have a slug, other times an id (and these is used as key and name in components). If we can standardise on one name for this id / slug (slug?) that might help simplify this.
There was a problem hiding this comment.
Good call on this. I looked for places where this was inconsistent and addressed (see ff5de1)
| // @todo similar logic is done in `blocks/cart-checkout/cart/full-cart/index.js` | ||
| // we should extract this to use in both places so it's consistent. | ||
| // @todo this will need to account for `DISPLAY_PRICES_INCLUDING_TAXES`. | ||
| /** |
There was a problem hiding this comment.
Agree we need an abstraction for cart totals/arithmetic!
I also think it's a good idea to add an explicit method for converting a money value to float, i.e. parseInt( money_value, 10) is used a lot. Though if we have a cart calculations module then perhaps that's not a big deal.
These @todo comments should have matching issues 📥
There was a problem hiding this comment.
These @todo comments should have matching issues 📥
See this comment
Once the reference pull is extracted into individual pulls and they are merged, my plan was to go through the todos and and create issues for them. Nearly all these todos are already noted in this reference pull comment explicitly created for tracking them in the interim. |
This pull continues the extraction process from #1780.
The focus of this pull was primarily around updating existing hooks and adding new hooks related to the new contexts and payment method interface. This also implements the changes and removes obsolete hooks. Broken down, this pull:
base/hooksdirectory to make things a bit more organized. Primarily moving hooks into their own topical folders (I didn't move existing tests yet, that can be done in subsequent pulls that increase test coverage).useStoreOrderwhich is a placeholder hook for orderId and order loading. It will be finished in a follow-up pull.useBillingDatahook which exposes the billing data interface from thePaymentMethodDataContextProvider.usePaymentMethodInterfacehook (and related typedefs) which exposes the interface from contexts that are fed to payment methods as props. Also implements this hook in the payment method components.useCheckoutSubmitanduseCheckoutRedirectUrlwhich both expose slices of the checkout context.To test
This impacts cart and checkout blocks so testing simply involves ensuring that: