-
Notifications
You must be signed in to change notification settings - Fork 687
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[feature]: refactor edit steps out of redux and into local checkout state #1338
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://venia-git-reduxrefactor-checkout.magento-research1.now.sh |
… editing and setEditing
a878590
to
623a0da
Compare
|
|
||
const handleSubmitAddressForm = useCallback( | ||
formValues => { | ||
submitShippingAddress({ | ||
async formValues => { |
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.
We had to make some of the callbacks async
so that they could appropriately handle an error response.
|
||
useEffect(() => { | ||
async function requestPaymentNonce() { | ||
try { | ||
const paymentNonce = await dropinInstance.requestPaymentMethod(); | ||
onSuccess(paymentNonce); | ||
} catch (e) { | ||
// If payment details were missing or invalid but we have data | ||
// from a previous successful submission, use the previous data. | ||
const storedPayment = storage.getItem('paymentMethod'); |
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.
This was a weird UX. If storage has a valid payment and I enter a new card but something is invalid (pin/expiry/etc), it'll just close the form and use the storage payment info and I would assume the new info was accepted. I don't think we should do that automatically, so I removed this code. In the future we could display some UX that shows a "edit" button next to saved CC info that would re-mount the braintree dropin but for now we just have to deal with re-entering CC info.
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.
Agreed it was a weird UX. If I remember correctly we were trying to avoid having to re-enter CC info if the only thing that was changing was the billing address.
I think you're right that even though re-entering CC info is cumbersome it is better than leaving the user uncertain or unknowledgeable about what CC info is really being used. I'd rather them have to be explicit.
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.
This looks good to me. I have small concerns about the async callbacks interacting with reducers, but we'll resolve them when we actually pull state out of Redux.
packages/venia-concept/src/components/Checkout/__tests__/form.spec.js
Outdated
Show resolved
Hide resolved
packages/venia-concept/src/components/Checkout/__tests__/form.spec.js
Outdated
Show resolved
Hide resolved
Uh idk why the tests are failing... |
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.
LGTM, this does simplify things. It may be a candidate for later abstraction and public API, and this makes that more achievable. Thank you!
Description
This is a followup to #1309. The original refactor left much of the original redux piping in place. This PR shows how we can remove parts of it relatively painlessly. Here we are migrating the state of what form is being edited, ie payment, address or shipping, into local form state.
I also removed some unnecessary props and added some safety checks/state to make sure we only enable the submission of billing information when appropriate.
Related Issue
Closes #1405
Verification Steps
Screenshots / Screen Captures (if appropriate)
Proposed Labels for Change Type/Package
Checklist: