Skip to content

Jetpack Onboarding: Save site type selection#21154

Merged
tyxla merged 2 commits intomasterfrom
update/jetpack-onboarding-site-type-step-save
Jan 4, 2018
Merged

Jetpack Onboarding: Save site type selection#21154
tyxla merged 2 commits intomasterfrom
update/jetpack-onboarding-site-type-step-save

Conversation

@tyxla
Copy link
Copy Markdown
Member

@tyxla tyxla commented Dec 29, 2017

This PR updates the site type step to be saved in the remote site when clicking the corresponding tile.

To test:

  1. Checkout this branch on your local Calypso.
  2. Make sure your JP sandbox is running REST API: JPO - save site type jetpack#8431.
  3. Go to https://YourJetpackSandbox.com/wp-admin/admin.php?page=jetpack&action=onboard&calypso_env=development where YourJetpackSandbox.com is the domain of your Jetpack sandbox.
  4. After you land the JPO flow, skip to the Site Type step.
  5. Click the "Personal" tile.
  6. Verify the jpo_site_type option was saved to personal (to check this, go to https://YourJetpackSandbox.com/wp-admin/options.php and look for the jpo_site_type option)
  7. Repeat steps 5-6 again, but this time with the Business tile, and business as an option value.

@tyxla tyxla added Jetpack Onboarding [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 29, 2017
@matticbot
Copy link
Copy Markdown
Contributor


class JetpackOnboardingSiteTypeStep extends React.PureComponent {
handleSiteTypeSelection = siteType => {
const { siteId } = this.props;
Copy link
Copy Markdown
Contributor

@ockham ockham Jan 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you ditched this line and instead just inlined this.props.siteId below, you could collapse the method definition, using some arrow fun:

handleSiteTypeSelection = siteType => () => {
	this.props.saveJetpackOnboardingSettings( this.props.siteId, {
		siteType,
	} );
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, addressed in ec21a1d.

Copy link
Copy Markdown
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'm just wondering -- do we even need to store the site type on the backend? Isn't this step simply relevant for the UI, to decide what the next step is?

@tyxla tyxla force-pushed the update/jetpack-onboarding-site-type-step-save branch from c256a59 to 15833a8 Compare January 3, 2018 13:21
@tyxla
Copy link
Copy Markdown
Member Author

tyxla commented Jan 3, 2018

I'm just wondering -- do we even need to store the site type on the backend? Isn't this step simply relevant for the UI, to decide what the next step is?

TLDR: yes - it would be useful to store it for cases when the user doesn't finish the onboarding flow in one breath, but returns later to complete it. Also, it seems the original plugin stores the site type, so we're only mirroring that functionality: https://github.com/Automattic/jetpack-onboarding/blob/master/class.jetpack-onboarding-end-points.php#L373

@tyxla
Copy link
Copy Markdown
Member Author

tyxla commented Jan 3, 2018

@ockham I intend to merge this one after Automattic/jetpack#8431 lands. Let me know if you have any other suggestions before we 🚢 .

Copy link
Copy Markdown
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 ahead! 😄

Copy link
Copy Markdown
Contributor

@AnnaMag AnnaMag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@tyxla tyxla 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 Jan 4, 2018
@tyxla tyxla merged commit 0298506 into master Jan 4, 2018
@tyxla tyxla deleted the update/jetpack-onboarding-site-type-step-save branch January 4, 2018 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants