-
Notifications
You must be signed in to change notification settings - Fork 683
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
[dev]: Refactor Checkout #1309
[dev]: Refactor Checkout #1309
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://venia-git-refactor-checkout.magento-research1.now.sh |
submit, | ||
submitting | ||
} = props; | ||
const formState = useFormState(); |
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.
A wild hook appears!
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.
Good comment. I'm not surprised, though; Informed implements change publishing manually, so they're not doing hooks + context in any sort of optimal way.
2d0957b
to
fc10741
Compare
} | ||
const isSubmitDisabled = (busy, valid) => busy || !valid; | ||
|
||
const EditableForm = props => { |
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.
The parent form is now split into multiple function components. Should we move these into a Form/ directory and split them out into individual files?
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.
@jimbo says he prefers flat structure. I like nested, for things that are specific to a parent and won't be shared. 🤷♂
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.
The parent form is now split into multiple function components. Should we move these into a Form/ directory and split them out into individual files?
We should definitely split them out into individual files, as per existing convention. I do like the idea of a Form/
directory personally - especially since we have a Receipt
directory within Checkout
already.
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.
@jimbo says he prefers flat structure. I like nested, for things that are specific to a parent and won't be shared. 🤷♂
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 should never have had a Receipt
folder either—last year we had a few contributors who didn't like to follow conventions. 😉
Honestly though, if you guys prefer to nest folders in places with a lot of files like this, I'm fine with it. Just don't go overboard.
a776cb4
to
f666c8b
Compare
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.
@sirugh Looks good. A couple remaining small items if you'd like, but this is basically ready. 👍
@jimbo feedback addressed! If this is good to go please put it in Ready for QA :) |
Description
Refactoring of checkout components, including updating unit tests. Notably:
SubmitButton
in favor of just using theButton
component directly.ResetButton
.useEffect
in the component and see how I had to use a local variable to ensure we teardown/error properly.informed
updates state ifinitialValues
are used causing multiple renders.Form/
directory and split them out into individual files?Related Issue
Closes #1294.
Verification Steps
How have YOU tested this?
Lots of manual testing through different scenarios. See above.
Screenshots / Screen Captures (if appropriate):
Proposed Labels for Change Type/Package
Checklist: