Guided Transfer: Save host details #7534
Conversation
jordwest
force-pushed
the
add/guided-transfer/save-host-details
branch
from
77c0918 to
55dd300
Compare
|
This PR is now complete and ready for review, but it includes and depends on the changes in #7430, so I'll wait until that's merged before changing the status here |
|
I had a quick test of this, and it redirected to the checkout page as expected. I won't dig much more until #7430 lands and the overlapping code is yanked out of this one. |
jordwest
force-pushed
the
add/guided-transfer/save-host-details
branch
from
cd83eb0 to
d75f8f6
Compare
|
#7430 has now landed, I've rebased this PR and marking it ready for review. |
jordwest
added
[Status] Needs Review
jordwest
force-pushed
the
add/guided-transfer/save-host-details
branch
from
52250b6 to
72e10e9
Compare
|
Rebased again and resolved conflicts introduced in #7270 |
|
I gave this a test, and it works as expected. The code all looks pretty reasonable to me, but I'm not very familiar with some of the things this code uses, so I'd like to get a proper review from a dev who's got fingers in Calypso code every day. |
| @@ -8,7 +8,7 @@ import { connect } from 'react-redux'; | |||
| * Internal dependencies | |||
| */ | |||
| import PostTypeOptions from './post-type-options'; | |||
| import SpinnerButton from './spinner-button'; | |||
| import SpinnerButton from 'components/button/spinner-button'; | |||
There was a problem hiding this comment.
Few issues with this change:
- Why not create a separate
components/spinner-button? - The contained
className(export-card__spinner-button) does not adhere towpcalypso/jsx-classname-namespace
There was a problem hiding this comment.
Moved this into components/spinner-button in 7c08267, and fixed up the className
jordwest
force-pushed
the
add/guided-transfer/save-host-details
branch
2 times, most recently
from
040f9b8 to
2ffb657
Compare
|
Thanks for the thorough reviews @aduth! I've made most of your suggested changes and responded inline |
| } | ||
|
|
||
| componentWillReceiveProps( nextProps ) { | ||
| if ( nextProps.isAwaitingPurchase === true ) { |
There was a problem hiding this comment.
What other truthy values are you expecting? We're usually not so explicit about these comparisons. Instead, this is just as effective:
if ( nextProps.isAwaitingPurchase ) {This adds a reducer to store the current host validation error code, and a local notice to display the error with a link to the contact form.
Removing a return statement in a previous commit caused the logic to fall through and dispatch the fail action even if the request succeeded and host_details_entered was true
jordwest
force-pushed
the
add/guided-transfer/save-host-details
branch
from
d235df2 to
05c6522
Compare
|
I've just rebased on master and fixed some minor issues (05c6522 and 1c31812). @rickybanister: I also switched the error notices to appear above the form in d9e05dd. @aduth: do you feel this needs further review or would you be comfortable giving it the thumbs up? |
| translate( "We've already confirmed your details. Please contact support if you need to change them." ), | ||
| }; | ||
|
|
||
| const errorText = get( |
There was a problem hiding this comment.
A case to be made for a switch with default here would be that you could avoid the translate call for the default value that you're always making here, since translate is a non-trivial function call. Not worried enough to warrant any change.
|
No other comments on the code, and the "invalid details" error showed for me as expected in testing. LGTM 👍 |
aduth
added
[Status] Ready to Merge
and removed
[Status] Needs Review
|
Thanks! I swapped out the |
Depends on #7430MergedThis changes triggers the save host details action (which hits the relevant API endpoint) when the Continue button is pressed on the host details form, then redirects to the checkout.
How to test
Test live: https://calypso.live/?branch=add/guided-transfer/save-host-details