Add client side postcode validation #8503
Conversation
nielslange
added
type: enhancement
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: +3.34 kB (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
assets/js/base/components/cart-checkout/address-form/address-form.tsx
Outdated
Show resolved
Hide resolved
assets/js/base/components/cart-checkout/address-form/address-form.tsx
Outdated
Show resolved
Hide resolved
| @@ -214,6 +214,47 @@ const AddressForm = ( { | |||
| ); | |||
| } | |||
|
|
|||
| const customValidationHandler = ( | |||
There was a problem hiding this comment.
I don't think it makes sense for this customValidationHandler to live here alongside other field types. It's only used by the postcode field. If you look at the ValidatedTextInput below there is also quite a bit of duplication I feel could be refactored more cleanly.
How about defining this elsewhere (outside of the AddressForm component/utility) and add this to the main component being returned so that customValidation is either postcode validation (if the field key is postcode) or undefined for other types?
There was a problem hiding this comment.
So like a general validation handler?
There was a problem hiding this comment.
Just something to improve the convenience because if we move to modal, the local state needs to be used over the data store.
| ) => { | ||
| if ( | ||
| ! isPostcode( { | ||
| postcode: values.postcode, |
There was a problem hiding this comment.
Are we using values state because inputObject.value only gives us postcode?
I wonder if the rest of the address state is something we should pass to customValidationHandler to make this type of validation easier. cc @senadir ?
There was a problem hiding this comment.
I'm not really sure, I think it make sense to only inputObject for now, as long as we're not using the address is some comparison or event tracking then it's fine for now.
There was a problem hiding this comment.
Are we using values state because inputObject.value only gives us postcode?
I was using values as the callback function for customValidation expects an HTMLInputElement as input.
I wonder if the rest of the address state is something we should pass to customValidationHandler to make this type of validation easier.
Within customValidationHandler(), values holds the following object:
{
"first_name": "",
"last_name": "",
"company": "",
"address_1": "",
"address_2": "",
"city": "111",
"state": "",
"postcode": "",
"country": "",
"phone": ""
}Is that what you meant?
| return true; | ||
| }; | ||
|
|
||
| if ( field.key === 'postcode' ) { |
There was a problem hiding this comment.
See my other comments. I think we can just adjust the existing ValidatedTextInput below this block of code rather than redefine it for postcode. All that changes is the customValidation.
nielslange
force-pushed
the
add/7722-add-client-side-postcode-validation
branch
from
ef08d98 to
711bddd
Compare
There was a problem hiding this comment.
Thank you for working this, @nielslange. I've added a few comments regarding the Indian postcodes. Let me know if you have any questions about it.
assets/js/base/components/cart-checkout/address-form/address-form.tsx
Outdated
Show resolved
Hide resolved
packages/checkout/utils/validation/getValidityMessageForInput.ts
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Mike Jolley <[email protected]>
| field.key === 'postcode' | ||
| ? newValue.trimStart().toUpperCase() | ||
| : newValue, |
There was a problem hiding this comment.
When the input field is postcode, then remove leading spaces and convert lowercase to uppercase characters on input. This part ensures that the client-side postcode validation of WooCommerce Blocks, is in sync with the server-side postcode validation of WooCommerce.
There was a problem hiding this comment.
@mikejolley Based on our internal discussion, I've updated the PR. Would you mind reviewing it again?
|
@nielslange formatting changes seem to work well 👍🏻 I am noticing something off however. If you fill out the entire form, valid, then change say "city" by typing a letter—do not click out of the field, just leave it. Note the push changes (shipping will update). The same is no longer happening for the postcode field for me. Can you confirm? |
There was a problem hiding this comment.
This is ok to merge now—I fixed the bug. You were using the values object for validating the postcode field, but this value is always out of date because validation runs before the change handler. I fixed it by using the value from the input itself.
Test case was to use a US address, enter 90210, then remove the 0. By logging the values passed to the validation handling before my fix it would validate "90210" and push. Now it validates "9021" and does not push because validation rightly fails.
|
Thanks for reviewing and applying a fix to this PR, @mikejolley! 🙌 |
Fixes #7722
Currently, we're validating the postcodes in the Checkout block using the PHP-based validation of WooCommerce core. This requires sending a request to the server to validate the postcode. This PR aims to validate the postcodes client-side using TS.
Alternative PR
Note
Due to a rebasing conflict, #7945 was automatically merged into
trunk. I removed the accidentally merged PR. This PR aims to add the client side postcode validation totrunk.Testing
Automated Tests
User Facing Testing
AA9A 9AApasses the validation.9999 999fails the validation.AA9A 9AA.aa9A 9aa, are automatically converted to uppercase letters, e.g.AA9A 9AA.WooCommerce Visibility
Changelog