Domain Contact Form: Implement New Phone Input AB Test #10447

umurkontaci · 2017-01-06T02:26:54Z

🎉 Fixes #10369.
The time has come.

This PR puts the new phone input to Domain Contact Form with 50% variation on AB test.

Teaser

Testing

Turn on the abtest domainContactNewPhoneInput.

Test 1

Sign up as a new user
Register a domain.
While the input is empty, change the country.
Ensure the country field changes in phone field.
Fill the phone input in your local format (national or international)
Change the country field again, the phone field should stay the same.
Register the domain
Add another domain to cart
Ensure the phone number field loads nicely.
Cancel the domain, remove the other domain from cart.

Test 2

Switch to an existing user with a domain
Add a domain to cart.
In contact form, ensure the phone number looks correct.
Remove the domain from cart.

/cc: @wensco @aidvu @klimeryk

matticbot · 2017-01-06T02:26:55Z

Test live: https://calypso.live/?branch=update/domain-contact-phone-input

klimeryk · 2017-01-09T16:26:19Z

client/my-sites/upgrades/checkout/domain-details-form.jsx

+					onChange={ this.handlePhoneChange }
+					{ ...omit( this.getFieldProps( 'phone' ), 'onChange' ) } />
+			);
+		} else {


You should just return here, without the else branch (it causes an eslint warning 🤷‍♂️).

Fixed this but honestly, it makes the code harder to read with a conditional return. I think we should change the linter to allow if-else with only one if block.

If the return line is long like here - I agree, it's not really helping with readability. But with simpler return values it's usually helpful.

klimeryk · 2017-01-09T17:50:49Z

client/my-sites/upgrades/checkout/domain-details-form.jsx

@@ -132,7 +158,7 @@ export default React.createClass( {
 			name,
 			ref: name,
 			additionalClasses: 'checkout-field',
-			value: formState.getFieldValue( this.state.form, name ),
+			value: formState.getFieldValue( this.state.form, name ) || '',


Is this change required by the new field? Maybe it should be fixed there (so that it properly returns an empty string when no value is input), instead of here?

I prefer to ensure data is in the correct format as early as possible. There's no point in passing null to a text field and it'll force the field to have null checks or default values.

Fair enough - I was thinking about primitive/native fields here, but more complex components might indeed need additional logic to handle such cases correctly 👍

klimeryk · 2017-01-09T18:02:39Z

Code looks good 👍

Tested:

Same country and phone country code - 👍 .
Different country and phone country code - 👍 (phone country code was used).
No phone, no country - selecting the country selects the phone's country (sweeeet!), unless the phone's country was explicitly selected. It'd be nice to have it the other way around, though. Since the phone number is the first thing that the user will fill out, then the country (at least going field by field), the phone's country could be used to prepopulate the country field, if it's empty.
The default, disabled, variation still works fine 👍

Some issues I found:

Got a warning with the new input field just after visiting the domain contact info form:
Unknown props 'countriesList', 'moment', 'numberFormat', 'translate' on <select> tag in select (created by FormCountrySelect)
I swear I'm bumping into these edge cases by mistake, but it seems I found another one (or a regression? Since Kosovo also triggers it):
If you select Pitcairn from the country list of the new input, you get a console error:
app:///./client/components/phone-input/index.jsx:68 Uncaught TypeError: Cannot read property 'replace' of undefined
This also happens when you select the empty entry between the common countries and the rest. Which should not be selectable to begin with - setting disabled on that option should fix this particular case.
Very weird bug: enabled variation. Input a phone number, for example 987654321 (the default country, US, is OK). Now, cause validation error somewhere else in the form (for example, input an invalid email address). Focus the error field, hit backspace - after the second press you'll start deleting... digits from the phone field XD Probably some refs logic gone bad (for focusing the error field?) or conflicting ids? Can't reproduce this with the disabled variation.
Probably unrelated to this PR, but also noticed an error when you try to submit the form without selecting the state (when it's mandatory):
app:///./client/my-sites/upgrades/checkout/domain-details-form.jsx:330 Uncaught TypeError: this.refs[(0 , _kebabCase3.default)(...)].focus is not a function
Breaks the layout a bit too:

Noticed that when the phone number is empty, the value sent to the backend is not empty, but contains just the country code, for example: "+1.". Even if no country was explicitly selected. But that makes sense - it suppresses the "missing country code" error. Just noting it here in case I missed something and it was not intended.

umurkontaci · 2017-01-17T01:46:04Z

Got a warning with the new input field just after visiting the domain contact info form:
Unknown props 'countriesList', 'moment', 'numberFormat', 'translate' on select tag in select created by FormCountrySelect.

Gonna fix this in another PR, I'm going to remove the dependency of FormCountrySelection. It shouldn't have any side effects to keep it this way though.

Very weird bug:

I can't really reproduce this, we only switch focus if you click submit. Can you give more instructions to reproduce?

this.refs[(0 , _kebabCase3.default)(...)].focus is not a function

Yep, It's unrelated and it's caused due to wrapping components with connect and localize. I have a solution reduxjs/react-redux#270 (comment) but that's for another PR as well.

I swear I'm bumping into these edge cases by mistake, but it seems I found another one (or a regression? Since Kosovo also triggers it):

Added Kosovo's dial code manually, there a few uninhabited countries we are allowing, I'm removing them at D3984-code.

Noticed that when the phone number is empty, the value sent to the backend is not empty, but contains just the country code, for example: "+1.".

That on its own shouldn't cause a problem, I think. We don't require country code so it makes sense to suppress the message.

klimeryk · 2017-01-18T04:13:20Z

I can't really reproduce this, we only switch focus if you click submit. Can you give more instructions to reproduce?

I could reliably reproduce that on Chromium 51. After updating to 57, it works correctly. Can't reproduce it in Safari either, so I'd consider this a bug in Chromium.

As for everything else - sounds/looks good to me ;) (apart from the broken tests - but that should be fixable quickly)
Not sure what your plans are for merging this, but could we merge #10443 first and test this again?

umurkontaci · 2017-01-18T05:13:57Z

Since #10443 shifts a lot of stones I'm afraid introducing a regression and pushing the timeline further. I think this PR is much more urgent than #10443, I would rather get this in, start collecting data and merge #10443 after the test is over (should take a couple of days only). What do you think?

klimeryk · 2017-01-18T05:23:39Z

True, but I'd rather shift the timeline again, but test the final-ish version than to test this one, merge the fixes (when? after the a/b test finishes so as to not impact it?) and find out after rolling it out to everyone that it introduced a regression :/ Believe me when I tell you I want this out the door as much as you do 😀 But if we're making such big changes as in #10443, I'd rather introduce them before the A/B testing, not after.

umurkontaci · 2017-01-18T23:11:06Z

Merged #10443.

Please test rigorously :)

This still depends on #10759, but that one should be easy to get in.

klimeryk

Not a blocker, but when tabbing between fields in the form, the phone's country list is not focused properly (you know, the one with the flag). It might be confusing for some users - maybe we can remove it from the tabbing order? (tabindex="-1" should do the trick)

You can still select the empty option between the popular countries and the rest (I swear I already reported this 🤔) - it will trigger a console error and the flag will be loading indefinitely.

Interesting bug (yaaaay 😕): start with a user that has no country pre-selected. Select a country (say, Poland). This will trigger to phone country to be Poland too (which is fine, of course). Now input a phone number starting with 1 - this will change the phone's country to USA. I'm not sure if this is intended - I'd wager that there's at least one country that could have a local phone number starting with 1?

klimeryk · 2017-01-19T10:31:27Z

client/my-sites/upgrades/checkout/domain-details-form.jsx

-		wpcom.validateDomainContactInformation( fieldValues, domainNames, ( error, data ) => {
+		const allFieldValues = Object.assign( {}, fieldValues );
+		if ( abtest( 'domainContactNewPhoneInput' ) === 'enabled' ) {
+			allFieldValues.phone = toIcannFormat( allFieldValues.phone, countries[ this.state.phoneCountryCode ] );


this.state.phoneCountryCode is lowercase (see line 129), so this lookup fails (the keys in countries are uppercase).

klimeryk · 2017-01-19T10:37:16Z

client/my-sites/upgrades/checkout/domain-details-form.jsx

-		setDomainDetails( formState.getAllFieldValues( this.state.form ) );
+		const allFieldValues = Object.assign( {}, formState.getAllFieldValues( this.state.form ) );
+		if ( abtest( 'domainContactNewPhoneInput' ) === 'enabled' ) {
+			allFieldValues.phone = toIcannFormat( allFieldValues.phone, countries[ this.state.phoneCountryCode.toLowerCase() ] );


Similar problem as on line 92.

…is a NANPA country

umurkontaci · 2017-01-19T19:53:02Z

You can still select the empty option between the popular countries and the rest (I swear I already reported this 🤔) - it will trigger a console error and the flag will be loading indefinitely.

👍

Not a blocker, but when tabbing between fields in the form, the phone's country list is not focused properly (you know, the one with the flag). It might be confusing for some users - maybe we can remove it from the tabbing order? (tabindex="-1" should do the trick)

👍

nteresting bug (yaaaay 😕): start with a user that has no country pre-selected. Select a country (say, Poland). This will trigger to phone country to be Poland too (which is fine, of course). Now input a phone number starting with 1 - this will change the phone's country to USA. I'm not sure if this is intended - I'd wager that there's at least one country that could have a local phone number starting with 1?

👍

klimeryk · 2017-01-20T02:50:20Z

Tested, works and looks good 👍 Woohoo! 💃
One last comment - let's ditch the placeholder for this field. With this new field, it doesn't help at all. For most countries, 9876543210 is not a valid phone number, so the placeholder is not formatted in the local format. For US, it is - but it's actually confusing, since you can't input parenthesis or dashes. So the user can't replicate the format shown in the placeholder (we do that automatically for him, but he doesn't know that). Additionally, on every re-render we reformat the placeholder, introducing some performance impact (he field is re-rendered a lot when the user is inputing the number). A simple Phone placeholder should do the trick.

umurkontaci · 2017-01-20T22:44:44Z

One last comment - let's ditch the placeholder for this field. With this new field, it doesn't help at all. For most countries, 9876543210 is not a valid phone number, so the placeholder is not formatted in the local format. For US, it is - but it's actually confusing, since you can't input parenthesis or dashes. So the user can't replicate the format shown in the placeholder (we do that automatically for him, but he doesn't know that). Additionally, on every re-render we reformat the placeholder, introducing some performance impact (he field is re-rendered a lot when the user is inputing the number). A simple Phone placeholder should do the trick.

I'm not sure removing it altogether would help. I have something WIP that generates actual phone numbers but that's not going to make it here. Maybe @breezyskies has something to say?

klimeryk · 2017-01-20T22:54:48Z

Not altogether - put Phone there, just like it is for Postal code, First name, etc.
Right now, in best case scenario we show a formatted phone number, for example, for US: (987) 654-3210. But the user can't input this number - they can only input numbers (no spaces, dashes or parenthesis). This could leave them very confused. Worst case scenario, we show 9876543210 which is hardly a helpful placeholder. I thought the idea of this field was that the user shouldn't care about the formatting - that's the field's job. The user just has to input his/her phone number.

umurkontaci · 2017-01-20T22:57:21Z

I see your point, the field allows both formats, but sometimes more experienced users can actually try to understand what the field expects from them, like whether to input in international format or not. Not having a clue can arise confusion.

klimeryk · 2017-01-21T01:25:03Z

True, but my point is that this placeholder is not really helping with that. If you want them to know that the country dial code prefix is not needed then change the design so that it includes the selected country code:

(the prefix could be shown next to the flag, of course, or as a prefix to the input field)

And wasn't the whole point of this field that the user doesn't have to know what format they should input their phone numbers? They can include country codes or not - it handles both cases. As an (hopefully) experienced user, I usually start my phone number with with the country code, which will trigger the flag to change indicating that I'm on the right track. The placeholder would have not helped me at all. In fact, for Poland (and many other countries), it seems broken - it doesn't show how the valid phone number will be formatted at all (9876543210 vs 987 654 321, for example).

Not having a clue can arise confusion.

Completely agree - just don't see how the current form of the placeholder helps with that :)

umurkontaci · 2017-01-23T21:35:50Z

Switched placeholder to "Phone", we can look ways to improve it later.

Fixed the issue with IE flag icon scaling as well.

umurkontaci added Components [Feature Group] Emails & Domains Features related to email integrations and domain management. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 6, 2017

umurkontaci self-assigned this Jan 6, 2017

umurkontaci requested review from aidvu and klimeryk January 6, 2017 02:26

umurkontaci changed the title ~~Domain Contact Form: Implement New Phone Input AB Test (#10369)~~ Domain Contact Form: Implement New Phone Input AB Test Jan 6, 2017

umurkontaci mentioned this pull request Jan 7, 2017

Phone Input: Build a better phone number input #7453

Closed

6 tasks

klimeryk reviewed Jan 9, 2017

View reviewed changes

umurkontaci force-pushed the update/domain-contact-phone-input branch from f57161e to 0e54743 Compare January 9, 2017 16:32

klimeryk reviewed Jan 9, 2017

View reviewed changes

klimeryk added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 16, 2017

umurkontaci force-pushed the update/domain-contact-phone-input branch from 9aab8fb to 6f99a26 Compare January 17, 2017 01:33

umurkontaci added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Jan 17, 2017

umurkontaci mentioned this pull request Jan 17, 2017

Phone Input: Improve data flow #10443

Merged

umurkontaci force-pushed the update/domain-contact-phone-input branch 2 times, most recently from ab1211e to 8214d46 Compare January 17, 2017 22:53

umurkontaci force-pushed the update/domain-contact-phone-input branch from 8214d46 to ac01ef6 Compare January 18, 2017 04:53

umurkontaci mentioned this pull request Jan 18, 2017

Phone Inputs: Test branch #10755

Closed

umurkontaci force-pushed the update/domain-contact-phone-input branch from ac01ef6 to d60d4be Compare January 18, 2017 23:09

klimeryk reviewed Jan 19, 2017

View reviewed changes

umurkontaci added 8 commits January 19, 2017 11:52

Domain Contact Form: Implement New Phone Input AB Test (#10369)

0406ba6

Phone Input: Update Metadata and inject Kosovo

5c9c2bf

Phone Input: Update Metadata and inject UM, BV, TF, HM, PN, GS

086b91d

Phone Input: Fix uppercase differences

6df3a68

Domain Details Form: Fix capitalization issues with phone input

7279e8b

FormCountrySelect: Disable the option is the "code" is falsy

56b5286

PhoneInput: Set tabIndex for FormCountrySelect to -1

bf3841a

PhoneInput: Only guess when the first digit is 1 if selected country …

a24b73e

…is a NANPA country

umurkontaci force-pushed the update/domain-contact-phone-input branch from d60d4be to a24b73e Compare January 19, 2017 19:53

klimeryk added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 20, 2017

klimeryk approved these changes Jan 20, 2017

View reviewed changes

umurkontaci added 2 commits January 23, 2017 12:42

Update test dates and change placeholder to "Phone"

7087b9a

Phone Input: Fix IE flag icon (Fixes: #9382)

da2b056

umurkontaci merged commit 95af140 into master Jan 23, 2017

matticbot removed the [Status] Ready to Merge label Jan 23, 2017

alisterscott deleted the update/domain-contact-phone-input branch January 24, 2017 01:32

alisterscott mentioned this pull request Jan 24, 2017

Add 'domainContactNewPhoneInput' and default group Automattic/wp-e2e-tests#373

Merged

sirbrillig mentioned this pull request May 5, 2020

Rewrite PhoneInput to allow control with different formats (4) #41783

Merged

leonardost mentioned this pull request Jul 12, 2021

Contact form: Fix problem when inserting phone number that begins with same digits as country code #54477

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Domain Contact Form: Implement New Phone Input AB Test #10447

Domain Contact Form: Implement New Phone Input AB Test #10447

umurkontaci commented Jan 6, 2017 •

edited

Loading

matticbot commented Jan 6, 2017

klimeryk Jan 9, 2017

umurkontaci Jan 9, 2017

klimeryk Jan 9, 2017

klimeryk Jan 9, 2017

umurkontaci Jan 9, 2017

klimeryk Jan 10, 2017

klimeryk commented Jan 9, 2017

umurkontaci commented Jan 17, 2017 •

edited

Loading

klimeryk commented Jan 18, 2017

umurkontaci commented Jan 18, 2017

klimeryk commented Jan 18, 2017

umurkontaci commented Jan 18, 2017

klimeryk left a comment

klimeryk Jan 19, 2017

klimeryk Jan 19, 2017

umurkontaci commented Jan 19, 2017

klimeryk commented Jan 20, 2017

umurkontaci commented Jan 20, 2017

klimeryk commented Jan 20, 2017 •

edited

Loading

umurkontaci commented Jan 20, 2017

klimeryk commented Jan 21, 2017

umurkontaci commented Jan 23, 2017

Domain Contact Form: Implement New Phone Input AB Test #10447

Domain Contact Form: Implement New Phone Input AB Test #10447

Conversation

umurkontaci commented Jan 6, 2017 • edited Loading

Teaser

Testing

matticbot commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klimeryk commented Jan 9, 2017

umurkontaci commented Jan 17, 2017 • edited Loading

klimeryk commented Jan 18, 2017

umurkontaci commented Jan 18, 2017

klimeryk commented Jan 18, 2017

umurkontaci commented Jan 18, 2017

klimeryk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umurkontaci commented Jan 19, 2017

klimeryk commented Jan 20, 2017

umurkontaci commented Jan 20, 2017

klimeryk commented Jan 20, 2017 • edited Loading

umurkontaci commented Jan 20, 2017

klimeryk commented Jan 21, 2017

umurkontaci commented Jan 23, 2017

umurkontaci commented Jan 6, 2017 •

edited

Loading

umurkontaci commented Jan 17, 2017 •

edited

Loading

klimeryk commented Jan 20, 2017 •

edited

Loading