-
Notifications
You must be signed in to change notification settings - Fork 144
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
[Hooks integration] Checkout Payment Section + Checkout Confirmation Page #1075
[Hooks integration] Checkout Payment Section + Checkout Confirmation Page #1075
Conversation
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.
Comparing the current behaviours vs the original one..
packages/template-retail-react-app/app/pages/checkout/partials/payment.jsx
Show resolved
Hide resolved
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'll continue reviewing the other files.
|
||
const CheckoutConfirmation = () => { | ||
const {orderNo} = useParams() |
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's cool now the confirmation page is appending the order id to the url.
packages/template-retail-react-app/app/pages/checkout/confirmation.test.js
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/pages/checkout/partials/payment-form.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/pages/checkout/partials/payment-form.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/pages/checkout/util/checkout-context.js
Show resolved
Hide resolved
packages/template-retail-react-app/app/pages/checkout/partials/payment.jsx
Outdated
Show resolved
Hide resolved
…/payment.jsx Co-authored-by: Vincent Marta <vmarta@salesforce.com>
@@ -311,7 +311,7 @@ const Cart = () => { | |||
return <CartSkeleton /> | |||
} | |||
|
|||
if (!isLoading && !basket?.productItems?.length) { | |||
if (!basket?.productItems?.length) { |
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.
If we check loading here, we won't need to guard basket below
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.
loading is always false here since it is checked on line 310
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.
if it is false, it indicates that basketData should be defined, we technically don't need to guard the basket here. Guarding is a good way to make sure things don't break, but if we know we have guarded other cases, we should not throw in the ? too much. 🤔
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.
sounds good, removed the guard
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.
@alexvuong @kevinxh FYI it looks the change broke the cart page when the cart is empty.
http://localhost:3000/global/en-GB/cart
TypeError: Cannot read properties of undefined (reading 'length')
at Cart (http://localhost:3000/mobify/bundle/development/pages-cart.js:394:28)
This is a regression that was fixed in this PR: #1062
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.
ok... i'm just gonna revert the change in cart.js.... it's out of scope for this PR and i thought it's a small diff improvement. but turns out to be too much hassle
packages/template-retail-react-app/app/pages/checkout/confirmation.jsx
Outdated
Show resolved
Hide resolved
} | ||
|
||
const mockProducts = { | ||
data: [ | ||
const mockOrder = { |
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.
[nit] Can this mock data live elsewhere to keep the test file smaller? It's more than half the lines of the file!
packages/template-retail-react-app/app/pages/checkout/partials/payment.jsx
Show resolved
Hide resolved
packages/template-retail-react-app/app/pages/checkout/partials/payment.jsx
Show resolved
Hide resolved
packages/template-retail-react-app/app/pages/checkout/partials/payment.jsx
Show resolved
Hide resolved
packages/template-retail-react-app/app/pages/checkout/partials/payment.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/pages/checkout/partials/payment-form.jsx
Outdated
Show resolved
Hide resolved
…/payment-form.jsx Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
This PR converts the checkout payment section to use
commerce-sdk-react
hooks. This PR also includes the switch from OCAPI to SCAPI for checkout on retail react app.Important UX changes
There is a significant change in terms of how payment is handled on SCAPI. Apparently SCAPI no longer accepts
customer_payment_instrument_id
as an input, which means that shoppers will not be able to apply their saved payments to the basket. The saved payments feature on retail react app becomes meaningless, and we have decided to remove the following UI components:Changes: