-
Notifications
You must be signed in to change notification settings - Fork 833
Forms: move international phone input to production #44922
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
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 7 files. Only the first 5 are listed here.
1 file is newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
8af7637
to
829003f
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.
Few issues/questions from first round of testing. For this I was just testing this field as is, not backwards compatibility.
Country updates when as I type?
https://github.com/user-attachments/assets/bd3f18b1-387e-407c-934f-08f1a990b18a
Validation seems off. There's a triangle under the input with no message, and then the message on the bottom of the form says 'undefined'.
Width issues. I know this is already in discussion, but the country dropdown seems wide even on large screen. It becomes more notable if using columns or on mobile.
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.
66229c3
to
16e5db9
Compare
default: { | ||
type: 'string', | ||
}, | ||
onChangeDefaultCountry: { |
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.
Why is it necessary to pass a function in attributes? Is it interactivity API thing?
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 don't think it's necessary. But it's part of the context provided for the innerblocks. As such, I thought it would be a good idea to keep it declared as it's supposed to be, so one can track down where is it coming from. Unsure if removing it from declaration impacts functionality.
Why the move from |
cfcb293
to
356a86d
Compare
… disregard default value
019e629
to
69ff365
Compare
6d40c44
to
7bede4e
Compare
* adapt process to use two letter country code, derive state from it * remove debugging code * move country list to field-telephone * remove field-phone registration and handlers * move input-phone out of beta * move field-phone code into field-telephone, update block definition and deprecation/migration * use type phone for proper wrapper and type setup, render old field-telephone as usual * adapt css classes to both telephone and phone * use new label format for country dropdown * improve deprecation/migration eligibility * add migration test * changelog * add test for phone field * only render legacy phone field if showcountryselector is not present, disregard default value * fix mobile views with long country names * deprovision default when not using country selector on phone field * cleanup useless code * use the right field type for international phone number: phone * only change country when lib has a possible value * create new instance whenever country changes * use same bblock properties declaration on php and js * inherit letter spacing so shared styles take effect * add default supports to field * required labels are affecting the label height, use smaller font * keep mimicking input styles on wrapper * use more probe styles to make wrapper look like input * export and use when available: input height css style from probe * use line-height from probe * move phone-input out of beta after some not optimal rebase * change phone-input name
Proposed changes:
This PR moves all the code we used to develop the country dropdown selector for phone input back into legacy telephone field.
Other information:
Jetpack product discussion
None
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Migration test: run
jetpack test js packages/forms
to verify the migration test passes. Then...The main test here splits in 2:
see the new phone field option
see that old phone fields remain apparently untouched
Checkout trunk and create a post. Add 2 forms to it and add a phone field to each of them. Turn one of them into multi-step, save and publish. Both on editor and frontend nothing should have changed.
Now checkout this branch,
move/international-phone-input-to-telephone
, build/watch both packages/forms and plugins/jetpack and reload the editor.Again, things shouldn't have changed, but if you select the phone fields (the field, not the inputs) there should be a "Show country selector" toggle on the inspector sidebar, do not enable it yet.
Reload the frontend to see the phone field hasn't changed.
Do some modification on the post and save. Reload the frontend, still nothing should have changed.
Back to the editor, enable the "Show country selector" toggle (you can try this one by one or both at the same time), select a default country if you like, that's gonna be the default selected country on frontend load
Reload frontend to see the new country selectors attached to the phone field
when country selector toggle is disabled, basic validation occurs (numbers and plus sign allowed)
when country selector toggle is enabled, validation is done against libphonenumber, try writing/pasting international numbers, beginning with "+" or "00" (double zero).
try changing country after writing a valid/invalid number
overall check of the validation lib is appreciated, but don't go to edgy on the cases
submit and verify the value is the expected one