Guided Transfer: Add SiteGround host details #7430
Conversation
jordwest
force-pushed
the
add/guided-transfer/siteground-credentials
branch
2 times, most recently
from
285de85
to
77ea136
Compare
jordwest
added
[Status] Needs Review
| } | ||
| } | ||
|
|
||
| Bluehost.propTypes = { |
There was a problem hiding this comment.
Not a big deal but just wondering why this doesn't appear inside the class statement.
There was a problem hiding this comment.
Good point, now that we have ES7+ property initializers, I've moved back it into the class in 84dd99046f7bb892ee896e17a8a1c9f0df80f40b.
Previously, setting it outside the class was the only way to set the PropTypes on the class when using ES6 style classes instead of React.createClass.
jordwest
force-pushed
the
add/guided-transfer/siteground-credentials
branch
2 times, most recently
from
82b3c0e
to
84dd990
Compare
|
Thanks for the reviews @omarjackman. I've made a few changes and left inline comments |
| <DestinationURL | ||
| value={ fieldValues.wporg_url } | ||
| onChange={ onFieldChange( 'wporg_url' ) } | ||
| /> |
There was a problem hiding this comment.
To trail the /> or not to trail it. The're a bit inconsistent in the code above. I personally like the format
<SomeComponent
prop1="prop"
prop2="prop" />
|
@jordwest I added some more review and suggested not using |
jordwest
force-pushed
the
add/guided-transfer/siteground-credentials
branch
from
84dd990
to
695efbd
Compare
|
Thanks @omarjackman, I've fixed up the bits and pieces and left a comment about using |
|
It's not clear for an external reviewer how to test this. If we want that third opinion, we should update with clear testing steps. That said, I feel like the review here has served its purpose, forcing conscientious consideration of code that raised a pale yellow flag on the part of a reviewer. I'm ok with shipping this as-is and looping back re state/ref if we get a strongly held third opinion that we should update it. @omarjackman any objection to that approach? Would you otherwise offer a 🚢 if not for the uncertainty around state/refs? |
|
LGTM, I've got no objections. |
|
lets 🚢 |
This refactors account-info.jsx to support different host details forms and adds a form for SiteGround.
jordwest
force-pushed
the
add/guided-transfer/siteground-credentials
branch
from
89456df
to
8d3085f
Compare
Without this, a warning appears as the fields go from uncontrolled to controlled.
jordwest
removed
the
[Status] Needs Review
| @@ -29,7 +29,7 @@ export const Username = localize( props => | |||
| } ) }</FormLabel> | |||
| <FormTextInput | |||
| id="username" | |||
| value={ props.value } | |||
| value={ props.value || '' } | |||
There was a problem hiding this comment.
This seems like it should be the default value for the prop?
This refactors the Guided Transfer
account-info.jsxto support different host details forms for different hosts and updates the form for SiteGround.Before
After
Bluehost requires a username and password, while SiteGround requires a username and email address.
There is some duplication in the
BluehostandSiteGroundcomponents, but by moving the common fields tofields.jsxI think this finds a good balance between reusability and flexibility to alter the forms for different host requirements.To come in a future PR will be the destination site URL.Destination URL field added in 77ea136Test live: https://calypso.live/?branch=add/guided-transfer/siteground-credentials