-
Notifications
You must be signed in to change notification settings - Fork 830
Forms: add searchable country dropdown for phone number #45018
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 6 files. Only the first 5 are listed here.
2 files are newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
5f76e48
to
d99c473
Compare
@@ -314,7 +314,7 @@ | |||
word-break: normal; | |||
color: unset; | |||
opacity: 0.6; | |||
font-size: 85%; | |||
font-size: 72%; |
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 are we doing this in this PR?
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.
Basically because we have a lot of broken stuff when it comes to new selectors and animated and outlined styles. This is just a wee fix for the font size given to notched labels on those form styles.
I'll be adding more like this.
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.
Let's get generic fixes merged in separate PRs to isolate them for easier testing.
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.
Add some questions and suggestion! Thanks for working in this!
/** | ||
* Test for telephone field_renders with showcountryselector false | ||
*/ | ||
public function test_make_sure_telephone_field_renders_as_expected_with_showcountryselector() { |
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.
❤️
display: flex; | ||
max-width: 40%; | ||
// display: flex; | ||
// width: 40%; |
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.
Should we just remove this comment?
data-wp-on--keydown="actions.phoneComboboxKeydownHandler"> | ||
<div class="jetpack-combobox-options"> | ||
<template | ||
data-wp-each--filtered="context.filteredCountries" |
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.
NIT: indentation seems off.
import parsePhoneNumber, { AsYouType } from 'libphonenumber-js'; | ||
import { countries } from '../../blocks/field-phone/country-list'; | ||
import { countries } from '../../blocks/field-telephone/country-list'; |
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.
It looks like this list is currently not translated. Should we wait till that is the case before adding it to production?
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 translation comes from an internal method on the field class. This was so the translations can be provided as interactivity config:
$translated_countries = $this->get_translatable_countries();
$global_config = array(
'i18n' => array(
'countryNames' => $translated_countries,
),
);
wp_interactivity_config( 'jetpack/field-phone', $global_config );
58eab79
to
8407703
Compare
'jetpack-field', | ||
'jetpack-field-phone', | ||
'jetpack-field-telephone', | ||
width ? ` jetpack-field__width-${ width }` : '', |
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.
Without space, since clx takes care of it?
width ? ` jetpack-field__width-${ width }` : '', | |
width ? `jetpack-field__width-${ width }` : '', |
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.
Or I guess this would work too?
width ? ` jetpack-field__width-${ width }` : '', | |
{ `jetpack-field__width-${ width }`: width }, |
@@ -1,13 +1,16 @@ | |||
.jetpack-contact-form .jetpack-field.jetpack-field-phone { | |||
.jetpack-contact-form .jetpack-field.jetpack-field-phone, | |||
.jetpack-contact-form .jetpack-field.jetpack-field-telephone { |
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.
Both classes probably not needed anymore?
@@ -7,8 +7,11 @@ import save from './save'; | |||
const name = 'phone-input'; | |||
const settings = { | |||
apiVersion: 3, | |||
title: __( 'International Phone Input', 'jetpack-forms' ), | |||
description: __( 'A compound input for international phone numbers.', 'jetpack-forms' ), | |||
title: __( 'Phone Input', 'jetpack-forms' ), |
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.
title: __( 'Phone Input', 'jetpack-forms' ), | |
title: __( 'Phone input', 'jetpack-forms' ), |
Sentence case everywhere :-)
description: __( 'A compound input for international phone numbers.', 'jetpack-forms' ), | ||
title: __( 'Phone Input', 'jetpack-forms' ), | ||
description: __( | ||
'A compound input for phone numbers with international support.', |
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 compound input for phone numbers with international support.', | |
'Phone number input.', |
We could keep it simple?
8407703
to
aa403a3
Compare
…cky, needs testing
@simison Thanks for the thorough review! Super helpful!
Fair enough, I was about to ask about it. Removed.
I adjusted the position, I think it sits better now.
Position and dimensions are a bit tricky since it's
Good!
With the above fixes this now only affects mobile views, but desktop displays show a better layout. Added flex alignment to top for the extreme cases
Yes! Another tricky scenario. We're trying to inherit as much as possible, but sometimes the backgrounds and colors can come from different places, becoming even transparent when a simple div wrapper shows up (KeyboardShortcuts for example). I've adjusted a bit, but in the end we'll need to make some compromises.
As seen on the above screenshots, this is now inheriting from either the input background or the set styles.
Changed to a subtle "separator" border with neutral color. We can work on it, see if we can inherit it.
I don't think this is a good idea. I think the combobox can (and should) be more controlled. It's a floating UI element and while we want it to blend in, it can come with some surprises. Happy to explore, I'll leave some examples of edgy cases, the second one doesn't look so bad: ![]() ![]()
Seeing the screenshots above, was that what you meant?
I still fail to find a reliable var, we can use some of the accent colors (1 through 6) or mimic the buttons, which use a preset variable on top of another (
Fixed! But I wonder if you also meant we should apply the font size to the scrollable list, that would be hard to adjust (back to the absolute position/dimension topic). Maybe we can settle for using a percentage of the font-size set for the input? Like .74em?
Done! ![]() |
AE: __( 'United Arab Emirates', 'jetpack-forms' ), | ||
GB: __( 'United Kingdom', 'jetpack-forms' ), | ||
US: __( 'United States', 'jetpack-forms' ), | ||
UY: __( 'Uruguay', 'jetpack-forms' ), |
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.
es el mejor país
…)" This reverts commit fbffda8.
Phone field with country selector, second iteration.
Proposed changes:
This PR implements a searchable dropdown for the country selector on the phone field.
Working on the editor:
Screen.Recording.2025-09-05.at.16.14.53.mov
Working on the frontend:
Screen.Recording.2025-09-05.at.16.16.01.mov
NOTE: the default country setting on the inspector sidebar can remain as a secondary way of setting it or we can remove it all along. It doesn't look as nice, but as a setting it also makes sense to leave it there.
Other information:
Jetpack product discussion
None
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Test: