Janitorial: Refactor Domain Transfer components per code standards #16382

aidvu · 2017-07-19T21:04:28Z

Migrate to ES6.

Test:

Go to Transfer -> Transfer domain to another registrar
Try clicking all of the actions
Make sure the UI is not stuck in submitting state
Make sure all works as before

matticbot · 2017-07-19T21:04:29Z

Test live: https://calypso.live/?branch=update/refactor-transfer-my-sites-domains

aidvu · 2017-07-19T21:05:47Z

client/my-sites/domains/domain-management/transfer/transfer-out/unlocked.jsx

 					'Privacy Protection has been enabled.' ) );
 			} else {
-				notices.success( this.translate( 'We\'ve canceled your domain transfer. Your domain is now locked back.' ) );
+				notices.success( translate( 'We\'ve canceled your domain transfer. Your domain is now locked back.' ) );
 			}

 			if ( this.isMounted() ) {


This is not supported in ES6. Need to figure a way to fix this.

Occurs in locked/unlocked a few times. Chances are the lock is enabled/disabled and the parent component rerenders.

Worst case can add a var _isMounted and set it on mount/unmount for now. But I'd rather fix it differently.

https://facebook.github.io/react/blog/2015/12/16/ismounted-antipattern.html

This reverts commit 1684bfa.

klimeryk

Other than the unbound method and that translate call, everything else seems to work A-OK 👍

klimeryk · 2017-07-21T22:52:45Z

client/my-sites/domains/domain-management/transfer/transfer-out/icann-verification.jsx


-	handleClick() {
+	handleClick = () => {
 		this.setState( { submitting: true } );

 		resendIcannVerification( this.props.selectedDomainName, ( error ) => {
 			if ( error ) {
 				notices.error( error.message );


It'd be awesome if you convert the usage of these global, old-style notices to the Redux ones from state/notices/actions (that'd get us a step closer to resolving #3396).

Lemme do that in a different PR. I can fix #3396 with it.

Sure 👍 I was postponing fixing that one, because I misunderstood the scope of that issue originally (context: #15880 (comment)). But it should be pretty straightforward if it's just "reduxifying" them.

You mean "just". 😆 ❤️

I'm sure that will be a very straightforward and small PR 😏

klimeryk · 2017-07-21T22:54:21Z

client/my-sites/domains/domain-management/transfer/transfer-out/index.jsx

 				</Header>
 				{ this.renderSection() }
 			</Main>
 		);
-	},
+	}

 	goToEdit() {


This should be bound to this.

klimeryk · 2017-07-21T22:56:40Z

client/my-sites/domains/domain-management/transfer/transfer-out/locked.jsx

 			displayRequestTransferCodeResponseNotice( error, getSelectedDomain( this.props ) );
 		} );
-	},
-
-	handleTransferClick() {


Hah, good call.

klimeryk · 2017-07-21T22:59:03Z

client/my-sites/domains/domain-management/transfer/transfer-out/transfer-prohibited.jsx

 	<div>
 		<SectionHeader label={ translate( 'Transfer Domain' ) } />
 		<Card className="transfer-card">
 			<p>
-				{ translate(
+				{ this.props.translate(


This should actually stay translate, since it's a stateless component and this.props is probably undefined, but translate is nicely available via the destructured parameter.

Yeah, missed this one somehow, eh.

klimeryk · 2017-07-21T23:00:23Z

client/my-sites/domains/domain-management/transfer/transfer-out/unlocked.jsx

 			} else if ( hasPrivacyProtection ) {
-				notices.success( this.translate( 'We\'ve canceled your domain transfer. Your domain is now locked and ' +
+				notices.success( translate( 'We\'ve canceled your domain transfer. Your domain is now locked and ' +


Same comment as above about global notices. If you prefer not to do it in this PR, that's cool, but it's not much more work - just pass the actions to connect and replace the call here.

klimeryk · 2017-07-21T23:24:47Z

client/my-sites/domains/domain-management/transfer/transfer-out/icann-verification.jsx

-			submitting: false
-		};
-	},
+class IcannVerification extends React.Component {


Maybe we could re-use the new ICANN verification notice from domain management here?

Totally a different PR. :)

Hah, fair enough ;)

aidvu · 2017-07-21T23:36:23Z

@klimeryk thanks! Nice 👀 as always! Will fix those two and merge.

aidvu self-assigned this Jul 19, 2017

aidvu requested review from matthusby, hambai, delputnam and klimeryk July 19, 2017 21:04

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

aidvu commented Jul 19, 2017

View reviewed changes

aidvu added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 19, 2017

aidvu added 3 commits July 20, 2017 14:18

Refactor transfer components

175eae9

functional component arg passing

e40e112

call set state w/o isMounted

8d079f4

aidvu force-pushed the update/refactor-transfer-my-sites-domains branch from 0deea5b to 8d079f4 Compare July 20, 2017 12:22

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

when just requesting code there's no state change

4b970eb

aidvu added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jul 20, 2017

aidvu added 2 commits July 20, 2017 14:52

move mounted to a class variable

1684bfa

Revert "move mounted to a class variable"

c020116

This reverts commit 1684bfa.

klimeryk approved these changes Jul 21, 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 Jul 21, 2017

missed some spots

72637d3

aidvu merged commit e8ac97f into master Jul 24, 2017

aidvu removed the [Status] Ready to Merge label Jul 24, 2017

aidvu deleted the update/refactor-transfer-my-sites-domains branch July 24, 2017 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Janitorial: Refactor Domain Transfer components per code standards #16382

Janitorial: Refactor Domain Transfer components per code standards #16382

aidvu commented Jul 19, 2017 •

edited

Loading

matticbot commented Jul 19, 2017

aidvu Jul 19, 2017 •

edited

Loading

aidvu Jul 19, 2017

klimeryk left a comment

klimeryk Jul 21, 2017

aidvu Jul 21, 2017

klimeryk Jul 21, 2017

aidvu Jul 21, 2017 •

edited

Loading

klimeryk Jul 23, 2017

klimeryk Jul 21, 2017

klimeryk Jul 21, 2017

klimeryk Jul 21, 2017

aidvu Jul 21, 2017

klimeryk Jul 21, 2017

klimeryk Jul 21, 2017

aidvu Jul 21, 2017

klimeryk Jul 21, 2017

aidvu commented Jul 21, 2017

Janitorial: Refactor Domain Transfer components per code standards #16382

Janitorial: Refactor Domain Transfer components per code standards #16382

Conversation

aidvu commented Jul 19, 2017 • edited Loading

matticbot commented Jul 19, 2017

aidvu Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

aidvu Jul 21, 2017 • edited Loading

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

Choose a reason for hiding this comment

aidvu commented Jul 21, 2017

aidvu commented Jul 19, 2017 •

edited

Loading

aidvu Jul 19, 2017 •

edited

Loading

aidvu Jul 21, 2017 •

edited

Loading