Add/tld/fr extra fields dialog #14156

deBhal · 2017-05-17T10:37:33Z

This PR adds a form to support .FR extra fields for the for the domains checkout flow and contact detail editing flow as per #13890.

I've added the domains/cctlds feature flag, and the .FR form is behind that, so the most critical testing is that we don't break calypso with the flag off:
DISABLE_FEATURES=domains/cctlds make run

Note: that the back-end will only offer .FR domains to proxied a12s, or sandboxes with the store sandbox enabled, so this is not a very community friendly PR, sorry :(

After that, we want to see:

the checkout flow works: contact details -> .fr details (for .fr domains) -> maybe privacy dialog -> checkout
The appropriate fields being visible for registrant type and country of birth
the (token) tests run (and don't break the entire suite anymore): npm run test-client client/components/domains/registrant-extra-info (or even better npm run test-client)
the extra fields are passed to the server

Things that are not in this PR:

no sanitation or validation at the moment ( we'll do that after we reduxify)
no registration success (we need to fix the backend)
no remembered data on the .fr form ( needs backend + redux )
domain management integration (after redux)
more testing (after redux)

Video:

And passing that data to the server:

matticbot · 2017-05-17T10:37:34Z

Test live: https://calypso.live/?branch=add/tld/fr-extra-fields-dialog

umurkontaci · 2017-05-17T18:52:54Z

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

@@ -56,7 +59,8 @@ export default React.createClass( {
 			form: null,
 			isDialogVisible: false,
 			submissionCount: 0,
-			phoneCountryCode: 'US'
+			phoneCountryCode: 'US',
+			registrantExtraInfo: false


Is this always boolean? If so, do you think it'd make it easier to understand if we prefix it with has or needs? If not, I think we should initialize it to null or undefined instead of false

umurkontaci · 2017-05-17T18:56:20Z

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

+
+	handleFrSubmit( registrantExtraInfo ) {
+		this.setState( { registrantExtraInfo } );
+		this.closeDialog( 'fr' );


I think we never open a dialog named fr. Did you mean isFrDialogVisible?
I think it'd would make it more resilient to just create functions for every dialog (or use const) instead of relying on literal strings sprinkled everywhere.

deBhal · 2017-05-18T05:13:44Z

@mikeshelton1503: No interaction yet, but I think that screenshot implements your design if you wanted to have a look and give me design feedback, especially around the DOB.

deBhal · 2017-05-18T05:20:19Z

@umurkontaci: Thanks for catching the fr typo, but I think I need to rethink the logic there anyway. needsExtraRegistrantInfo() isn't going to work once we pre-fill data or add a back button, so I think what I'll need to do is have matching arrays of "needsDialogX:bool" (based on props) and "hasSubmittedDialogX: bool" (based on state).

a8ci18n · 2017-05-19T02:22:56Z

client/components/domains/registrant-extra-info/fr-form.jsx

+		return (
+			<div>
+				<Card className="registrant-extra-info__title-card">
+						{ this.props.translate( '.FR Registration' ) }


Hi! I've found a possible matching string that has already been translated 66 times:
translate( 'Registration' ) ES Score: 10
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

deBhal · 2017-05-19T12:51:44Z

Ok, I've reworked the extras form to be thinner, and moved it out of the dialog and into the details form.

I've also added some "steps" logic, which is probably re-inventing a wheel, but might nonetheless be useful in #12622 if we wanted to add the privacy dialog in as another step. I've updated the screenshot to a video of the flow. We definitely need a back button.

I think I've settled on how we should handle the extra details forms too, as described in the README. If we want them to be usable in both the domain details and checkouts, we need them to be super-thin, so what I'm going for now is just displaying some fields with error messages, and passing out the value in all the fields to the parent, and leaving validation, sanitation etc to them.

It should be reasonably clean once we get the contact details out of the react state and into the redux state.

yoavf · 2017-05-21T10:41:34Z

client/components/domains/registrant-extra-info/fr-form.jsx

+						checked={ 'individual' === this.state.registrantType }
+						name="registrantType"
+						onChange={ this.handleChangeEvent } />
+					<span>An individual</span>


Missing translate (a few additional occurrences below)

yoavf · 2017-05-21T10:45:03Z

client/components/domains/registrant-extra-info/fr-form.jsx

+				<FormFieldset>
+					<FormLabel>
+						{ this.props.translate( "Who's this domain for?" ) }
+					</FormLabel>


Closing FormLabel should be at the end of the FormRadio element

a8ci18n · 2017-05-22T07:53:43Z

client/components/domains/registrant-extra-info/fr-form.jsx

+							checked={ 'individual' === this.state.registrantType }
+							name="registrantType"
+							onChange={ this.handleChangeEvent } />
+						<span>{ this.props.translate( 'An individual' ) }</span>


Hi! I've found a possible matching string that has already been translated 7 times:
translate( 'Individual' ) ES Score: 13

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

deBhal · 2017-05-23T01:52:52Z

There's no way to translate the content field in a css after rule, is there? ( 047a536 )

a8ci18n · 2017-05-23T01:57:17Z

client/components/domains/registrant-extra-info/fr-form.jsx

+							name="registrantType"
+							checked={ 'individual' === this.state.registrantType }
+							onChange={ this.handleChangeEvent } />
+						<span>{ this.props.translate( 'An individual' ) }</span>


Hi! I've found a possible matching string that has already been translated 7 times:
translate( 'Individual' ) ES Score: 13

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

deBhal · 2017-05-23T06:49:45Z

Update: It's alive!

All the fr-form fields are hooked up to the form's state, and in turn to contact details, which are then passed on to the purchase and validate endpoints, huzzah!:

The fields on the FR form show only if they're appropriate, and the organization guess is in place too (if you put something in the organization field in the main form the fr form defaults ot organization).

It doesn't actually work yet because the backend just ignores the extra fields right now, but we're nearly there!

Validation and sanitation don't work, and there is some unpleasant hardwiring in there, but I'd like to punt all that to a later PR and solve it by reduxifying everything. Showing different forms and passing values and errors back and forwards is going to be so much easier with the data in a redux state (it's been getting a bit painful here. On the upside, I understand a bug I've seen a few times now where the state of components fail to show the most recent input event).

It could use another look over, but It's getting late, so I'm going to mark this as "for review" now, just be on the lookout for stupidity.

deBhal · 2017-05-23T06:57:17Z

On the design front, I Italicized the 'Optional', and the date entry needs a look

blowery · 2017-05-24T01:14:56Z

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

+		}
+	}
+
+	render = () => {


render doesn't need to be bound to the instance

blowery · 2017-05-24T01:15:36Z

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

 	handleCheckboxChange = () => {
 		this.setPrivacyProtectionSubscriptions( ! this.allDomainRegistrationsHavePrivacy() );
 	}

-	closeDialog = () => {
-		this.setState( { isDialogVisible: false } );
+	closeDialog( dialogKey = 'isDialogVisible' ) {


these might need to be bound to the instance like they were before, since they're using this. just depends on how they're called..

Good catch! They do need to be bound to work when passed in to the PrivacyProtection component here

blowery · 2017-05-24T01:16:22Z

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

 		};
 	}

-	componentWillMount() {
+	componentWillMount = () => {


this doesn't need to be bound

klimeryk · 2017-05-24T17:58:02Z

client/components/domains/registrant-extra-info/fr-form.jsx

+		this.setState( { [ event.target.name ]: event.target.value } );
+	}
+
+	shouldComponentUpdate = ( nextProps, nextState ) => {


I think React.PureComponent does basically the same :)

Nearly, but not quite :( isEqual() is a deep comparison, and PureComponent uses a shallow one https://facebook.github.io/react/docs/react-api.html#react.purecomponent.

I had to add this to break an infinite rerender caused by passing the state up out of this component and adding it to the state of it's parent. This was the point at which I decided that I had forced this too far, and need to stop and reduxify it all :)

Ah, didn't realize it was deep comparison 👍

deBhal · 2017-05-24T21:43:28Z

client/components/domains/registrant-extra-info/fr-form.jsx

+			this.props.onStateChange( this.state );
+	}
+
+	getRelevantFields( state ) {


@hewsut: what does getRelevantFields do? I mean, I understand what it does inside the function. But I don’t see it called anywhere?

Nice catch! It captures the way the .FR fields depend on each other. I was using it on the state before passing it out at one point, but now I'm not sure - maybe we want to save everything to the database? At any rate, we will need it to handle validation (we don't care about errors in the the organization fields for individuals), but it's not being used right now. I should probably pull it out, but I don't think I will just yet unless someone objects strongly.

mikeshelton1503 · 2017-05-25T03:11:09Z

@deBhal Finally taking a look here. It's looking good, nice work!

I noticed a few things that need some more attention:

The Section Header for the additional step should read ".FR Registration" (or whatever the ccTLD is in the particular case).
For .fr, the country of birth field was set to Australia for me. It should either be empty or default to France.
I know @ranh suggested removing it but I'd still like to have the intro header and paragraph per the design in Domains: Adding .de, .ca, .fr ccTLDS #13890. We can tweak the copy some more but I do believe that some explanation is necessary. Also as @deBhal suggested it might be helpful to include a link to a support page (though generally I like to avoid links in a purchase flow).

Other than that, I'll work on updating the presentation of the date fields and there are couple other minor design improvements I'm going to make. I'll work on those changes tomorrow (Thursday).

Also, just a note, I did my best to research what a VAT, SIREN, SIRET, and EU Trademark Number should look like for the placeholder text, which is what you have implemented thus far. However, we should probably have someone more familiar with those numbers review.

a8ci18n · 2017-06-01T01:29:07Z

client/components/domains/registrant-extra-info/fr-form.jsx

+							id="registrantType"
+							checked={ 'individual' === registrantType }
+							onChange={ this.handleChangeEvent } />
+						<span>{ translate( 'An individual' ) }</span>


Hi! I've found a possible matching string that has already been translated 7 times:
translate( 'Individual' ) ES Score: 13

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-06-01T01:29:08Z

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

-		if ( needsOnlyGoogleAppsDetails ) {
+		// TODO: gather up tld specific stuff
+		if ( this.state.currentStep === 'fr' ) {
+			title = this.props.translate( '.FR Registration' );


Hi! I've found a possible matching string that has already been translated 66 times:
translate( 'Registration' ) ES Score: 10
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-06-01T02:11:56Z

client/components/domains/registrant-extra-info/fr-form.jsx

+							id="registrantType"
+							checked={ 'individual' === registrantType }
+							onChange={ this.handleChangeEvent } />
+						<span>{ translate( 'An individual' ) }</span>


Hi! I've found a possible matching string that has already been translated 7 times:
translate( 'Individual' ) ES Score: 13

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-06-01T02:11:57Z

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

-		if ( needsOnlyGoogleAppsDetails ) {
+		// TODO: gather up tld specific stuff
+		if ( this.state.currentStep === 'fr' ) {
+			title = this.props.translate( '.FR Registration' );


Hi! I've found a possible matching string that has already been translated 66 times:
translate( 'Registration' ) ES Score: 10
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-06-01T02:26:06Z

client/components/domains/registrant-extra-info/fr-form.jsx

+							id="registrantType"
+							checked={ 'individual' === registrantType }
+							onChange={ this.handleChangeEvent } />
+						<span>{ translate( 'An individual' ) }</span>


Hi! I've found a possible matching string that has already been translated 7 times:
translate( 'Individual' ) ES Score: 13

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-06-01T02:26:06Z

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

-		if ( needsOnlyGoogleAppsDetails ) {
+		// TODO: gather up tld specific stuff
+		if ( this.state.currentStep === 'fr' ) {
+			title = this.props.translate( '.FR Registration' );


Hi! I've found a possible matching string that has already been translated 66 times:
translate( 'Registration' ) ES Score: 10
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

@mikeshelton1503

New functionality is behind domains/cctlds feature flag Thanks @mikeshelton1503, @klimeryk, @blowery, @umurkontaci, @yoavf!

a8ci18n · 2017-06-01T04:23:33Z

client/components/domains/registrant-extra-info/fr-form.jsx

+								value={ dobMonths }
+								max="2"
+								type="number"
+								placeholder={ translate( 'MM', {


Hi! I've found a possible matching string that has already been translated 11 times:
translate( 'mm', { context: 'millimeters'} ) ES Score: 11

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-06-01T04:23:34Z

client/components/domains/registrant-extra-info/fr-form.jsx

+							id="registrantType"
+							checked={ 'individual' === registrantType }
+							onChange={ this.handleChangeEvent } />
+						<span>{ translate( 'An individual' ) }</span>


Hi! I've found a possible matching string that has already been translated 7 times:
translate( 'Individual' ) ES Score: 13

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-06-01T04:23:34Z

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

-		if ( needsOnlyGoogleAppsDetails ) {
+		// TODO: gather up tld specific stuff
+		if ( this.state.currentStep === 'fr' ) {
+			title = this.props.translate( '.FR Registration' );


Hi! I've found a possible matching string that has already been translated 66 times:
translate( 'Registration' ) ES Score: 10
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

deBhal · 2017-06-01T04:28:50Z

@klimeryk: I've implemented that final round of feedback and then squashed for the merge, which makes having a quick look pretty hard, so I've pushed add/tld/fr-extra-fields-dialog-backup-history (#). No point leaving it lying around, though, so I'll delete it tomorrow if you don't do it first :)

Bullet points Fixes:

method binding
example es6
placeholder localization

I've also dropped the unused code (into a local branch), the unnecessary check and the max and displayName.

Thanks again for such a detailed review!

I've rebased, squashed, and done a final check of everything, so I'm going to merge now.

klimeryk · 2017-06-01T17:23:58Z

client/components/domains/registrant-extra-info/docs/example.jsx

+ */
+import Card from 'components/card';
+import ExtraInfoFrForm from 'components/domains/registrant-extra-info/fr-form';
+import { forDomainRegistrations as getCountries } from 'lib/countries-list';


klimeryk · 2017-06-01T17:28:45Z

Congrats on merging this 😀 I've looked through the final version and it looked good - thanks for bearing with me! 🙇

deBhal · 2017-06-02T09:42:21Z

thanks for bearing with me! 🙇

I think you might have that backwards :) I really appreciate such a thoughtful review.

deBhal added [Feature Group] Emails & Domains Features related to email integrations and domain management. [Status] In Progress [Tests] Includes Tests [Type] Enhancement labels May 17, 2017

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label May 17, 2017

umurkontaci reviewed May 17, 2017

View reviewed changes

deBhal force-pushed the add/tld/fr-extra-fields-dialog branch from 4ba33a4 to 0664ca4 Compare May 18, 2017 05:01

a8ci18n reviewed May 19, 2017

View reviewed changes

deBhal force-pushed the add/tld/fr-extra-fields-dialog branch from 3e061c3 to 94423dc Compare May 19, 2017 12:30

yoavf reviewed May 21, 2017

View reviewed changes

a8ci18n reviewed May 22, 2017

View reviewed changes

a8ci18n reviewed May 23, 2017

View reviewed changes

deBhal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels May 23, 2017

deBhal force-pushed the add/tld/fr-extra-fields-dialog branch from c765019 to c14d093 Compare May 23, 2017 21:24

blowery reviewed May 24, 2017

View reviewed changes

klimeryk reviewed May 24, 2017

View reviewed changes

deBhal commented May 24, 2017

View reviewed changes

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 May 31, 2017

mhsdef mentioned this pull request May 31, 2017

Add/domain management redux base #14549

Closed

a8ci18n reviewed Jun 1, 2017

View reviewed changes

deBhal removed [Tests] Includes Tests [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Jun 1, 2017

a8ci18n reviewed Jun 1, 2017

View reviewed changes

deBhal force-pushed the add/tld/fr-extra-fields-dialog branch from 8fd73ed to bcd16c0 Compare June 1, 2017 02:20

a8ci18n reviewed Jun 1, 2017

View reviewed changes

Add Form and suporting logic for .FR domains extra registrant details

16a7b3b

New functionality is behind domains/cctlds feature flag Thanks @mikeshelton1503, @klimeryk, @blowery, @umurkontaci, @yoavf!

deBhal removed the [Status] Needs Rebase label Jun 1, 2017

deBhal force-pushed the add/tld/fr-extra-fields-dialog branch from bcd16c0 to 16a7b3b Compare June 1, 2017 04:18

a8ci18n reviewed Jun 1, 2017

View reviewed changes

deBhal closed this Jun 1, 2017

deBhal reopened this Jun 1, 2017

matticbot removed the [Status] Ready to Merge label Jun 1, 2017

deBhal merged commit cae2c1a into master Jun 1, 2017

klimeryk reviewed Jun 1, 2017

View reviewed changes

yoavf mentioned this pull request Jun 4, 2017

.FR extra fields form still show if .fr domain removed from cart #14748

Closed

klimeryk deleted the add/tld/fr-extra-fields-dialog branch June 5, 2017 15:19

yoavf added this to the ccTLDs: Registration & Privacy in Registration milestone Jun 12, 2017

yoavf added this to Done in ccTLDs Jul 5, 2017

Add/tld/fr extra fields dialog #14156

Add/tld/fr extra fields dialog #14156

Conversation

deBhal commented May 17, 2017 • edited

matticbot commented May 17, 2017

umurkontaci May 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deBhal commented May 18, 2017

deBhal commented May 18, 2017

Choose a reason for hiding this comment

deBhal commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deBhal commented May 23, 2017 • edited

Choose a reason for hiding this comment

deBhal commented May 23, 2017 • edited

deBhal commented May 23, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeshelton1503 commented May 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deBhal commented Jun 1, 2017 • edited

Choose a reason for hiding this comment

klimeryk commented Jun 1, 2017

deBhal commented Jun 2, 2017

deBhal commented May 17, 2017 •

edited

umurkontaci May 17, 2017 •

edited

deBhal commented May 23, 2017 •

edited

deBhal commented May 23, 2017 •

edited

deBhal commented May 23, 2017 •

edited

deBhal commented Jun 1, 2017 •

edited