-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Signup: Fix site
path through domain-first
flow
#11601
Conversation
@@ -109,7 +109,7 @@ function createSiteWithCart( callback, dependencies, { | |||
if ( ! user.get() && isFreeThemePreselected ) { | |||
setThemeOnSite( addToCartAndProceed, { siteSlug, themeSlugWithRepo } ); | |||
} else if ( user.get() && isFreeThemePreselected ) { | |||
fetchSitesAndUser( siteSlug, setThemeOnSite.bind( this, addToCartAndProceed, { siteSlug, themeSlugWithRepo } ) ); | |||
fetchSitesAndUser( siteSlug, setThemeOnSite.bind( null, addToCartAndProceed, { siteSlug, themeSlugWithRepo } ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of using an arrow function here?
fetchSitesAndUser( siteSlug, () => {
setThemeOnSite( addToCartAndProceed, { siteSlug, themeSlugWithRepo } );
} );
This may be just me but I find it more readable :).
The code looks good - I only had a minor question. However I've tested this pull request in an incognito tab several times but was sometimes redirected to the I have the feeling (maybe wrong) that there is a different behavior induced by some A/B tests : it worked fine every time I was presented with the option to upload a theme. |
5ff511d
to
7ceae43
Compare
@stephanethomas Thanks for pointing that out. I was able to reproduce this by throttling to "Good 3G" in Chrome. Unfortunately, this affects I added a commit to fix it, which was rather simple: ensure that we aren't setting the selected site in |
@@ -43,7 +43,7 @@ function createCart( callback, dependencies, data ) { | |||
|
|||
SignupCart.addToCart( cartKey, [ domainItem ], error => callback( error, providedDependencies ) ); | |||
} else { | |||
createSiteWithCart( callback, dependencies, data ); | |||
createSiteWithCart( callback, dependencies, data, reduxStore ); | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that we also have createSiteWithCartAndStartFreeTrial
function that calls createSiteWithCart
but reduxStore
param is not passed. I have no idea if that is required, but we should double check if it doesn't break anything.
I had one comment about missing |
@gziolo Addressed that, thx for noticing! |
53e3b95
to
5a69d5f
Compare
Product 👍 |
aca2949 updated
flow-controller
such that the redux store can be accessed in a step'sapiRequestFunction
viathis._reduxStore
, asapiRequestFunction
is bound to the context ofSignupFlowController
which has a_reduxStore
property.This is a little bit fragile, as:
StepActions
that calls another function will need to useFunction.prototype.call
to preserve thethis
value.I didn't catch this behavior when reviewing #10936, which broke the
domain-first
flow (which isn't public yet) if you select to create a site, as it called a function inStepActions
without using.call
to preserve theSignupFlowController
context.As far as I can tell, there isn't any reason to access
reduxStore
from the context instead of through a parameter, and doing the latter doesn't have the issues listed above. This PR makes that change.Testing
Visit
/start/domain-first
.Select a domain.
Select to create a site in the second step.
Proceed through the flow and assert that you land in checkout with the domain/plan you selected.
Code
Product