Split Checkout block component into smaller files #3062
Conversation
Aljullu
added
status: needs review
type: refactor
|
Size Change: +294 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
|
Proactive refactoring, I like! 🏅 |
There was a problem hiding this comment.
Thanks for streamlining this @Aljullu - great to see our code getting tidier and easier to wrangle as it grows.
I did some quick smoke testing in Chrome, Safari and Firefox, with Storefront. Didn't see any issues.
I did get confused by a quirk with Phone field (unrelated to this PR) - if the order is virtual, there's no Shipping address field, and no Phone field. I understand why this is the case – phone is a shipping field (not included in billing addr). But the block options aren't clear, so this could be confusing. Phone and Require phone number only apply to shippable orders. I haven't added a follow up issue since this might be expected behaviour for most merchants – do you think this is worth adding an issue for?
| 'wc-block-components-checkout-form', | ||
| className | ||
| ) } | ||
| className={ classnames( 'wc-block-components-form', className ) } |
There was a problem hiding this comment.
Just noting this change - not just a refactor, this changes the classname for form (and others). Is there any potential theme impact? Is this form component abstract or is it somewhat coupled to checkout?
There was a problem hiding this comment.
e.g. is this NumberedForm - where are the checkout-specific styles/behaviour (vs generic)?
There was a problem hiding this comment.
I assumed this is superseded by #3062 (comment), correct? 🙂
There was a problem hiding this comment.
Is this form component abstract or is it somewhat coupled to checkout?
e.g. is this NumberedForm - where are the checkout-specific styles/behaviour (vs generic)?
I assumed this is superseded by #3062 (comment), correct? 🙂
Yes ish - though I am still curious how generic this Form component is, might need more work/tweaks when used elsewhere (not in Checkout). We can iterate when that happens, if it is an issue.
| showPhoneField: PropTypes.bool.isRequired, | ||
| }; | ||
|
|
||
| export default CheckoutForm; |
There was a problem hiding this comment.
I see this is the business end, still a big component. Good to start breaking it down, and we can iterate more in future 😄
There was a problem hiding this comment.
Yeah, at some point it might make sense to keep splitting this one into smaller chunks. But I feel now it's much more self-contained (it's only the form) than before (it was the entire block inside a single component).
| ) } | ||
| </CheckoutForm> | ||
| <CheckoutForm | ||
| loginToCheckoutUrl={ loginToCheckoutUrl } |
There was a problem hiding this comment.
Do we need to pass the login URL through all the layers? Could be local to the component that needs it, or refactored out to utils/settings.
There was a problem hiding this comment.
Ah, good point. Moved it to a utils.js file in bc03b89.
| @@ -45,4 +45,8 @@ WC Blocks 3.3.0, introduced express payment methods in the Cart block. In order | |||
| <td><code>wc-block-components-express-checkout-payment-event-buttons</code></td> | |||
| <td><code>wc-block-components-express-payment__event-buttons</code></td> | |||
| </tr> | |||
| <tr> | |||
| <td><code>wc-block-components-checkout-form</code></td> | |||
| <td><code>wc-block-checkout__form</code></td> | |||
There was a problem hiding this comment.
Ah I see this change is documented! 👌
There was a problem hiding this comment.
Yes, and moved it to a specific file for 3.4.0 in 0710e73.
|
Spotted a visual issue in editor when reviewing this: #3068 |
|
Thanks for the review @haszari!
Good catch! I don't have a strong opinion on this, it's true it might be confusing for stores that only sell digital products, but I think that can be considered the expected behavior? |
Yep - agreed. Just noting the potentially confusing UX/behaviour when I see it 😄 |
The component for the Checkout block kept growing and growing as we added features, it was over 440 lines of code and it was becoming difficult to work with. This PR splits it into smaller files.
Even though the line diff of this PR is quite big, it doesn't add or remove any lines other than moving files/code around, renaming some CSS classes and updating the imports according to the new file structure.
How to test the changes in this Pull Request:
Do some smoke testing of the Checkout block: add it in the editor, change some attributes, verify it works in the frontend, try making a purchase, etc.