Skip to content
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

Jetpack Onboarding: Add country field to business address step #22580

Merged
merged 2 commits into from
Feb 20, 2018

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Feb 19, 2018

This PR adds a country field to the business address step. It also updates the settings schema to respect that field. Suggested in p6TEKc-1Rd-p2 and #22530.

Fixes #22530.

Preview:

To test:

  • Checkout this branch
  • Checkout the Jetpack branch from REST API: Add Country field to Business Address onboarding step jetpack#8874 on your Jetpack site.
  • Start the onboarding flow for the Jetpack site by going to /wp-admin/admin.php?page=jetpack&action=onboard&calypso_env=development
  • On the site type step, choose Business
  • Skip to the Business Address step.
  • Verify the country field appears properly.
  • Input some data in it, and save the step.
  • Verify the country saves properly and goes straight to the address widget.
  • Verify a fresh load of this step loads the setting properly.

@tyxla tyxla added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Task Jetpack Onboarding labels Feb 19, 2018
@tyxla tyxla self-assigned this Feb 19, 2018
@tyxla tyxla requested review from ockham and AnnaMag February 19, 2018 10:09
@matticbot
Copy link
Contributor

Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

Worked great! Leaving and observation but it's not exclusive to this PR. :shipit:

@oskosk
Copy link
Contributor

oskosk commented Feb 19, 2018

The changes here worked Great.

image.

I could also confirm that refreshing the page loaded the proper data for the country input

Just an observation. As soon as I got into the business addres form the first time, all of the inputs where red but this is not coming from this PR of course.

screen shot 2018-02-19 at 11 41 21 am

Copy link
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 👍

@AnnaMag AnnaMag closed this Feb 20, 2018
@AnnaMag AnnaMag reopened this Feb 20, 2018
@AnnaMag
Copy link
Contributor

AnnaMag commented Feb 20, 2018

☝️ oops, apologies for accidental button click.

@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 20, 2018
@AnnaMag
Copy link
Contributor

AnnaMag commented Feb 20, 2018

@oskosk, red fields are an indication of a required input, which makes sense if no address has been provided. This should be filled-in and green when the step had been completed before.
Would you expect to see a different view?

@tyxla tyxla force-pushed the update/jpo-business-address-add-country-field branch from f9c6dc4 to e57db06 Compare February 20, 2018 11:21
@@ -55,6 +56,7 @@ class JetpackOnboardingBusinessAddressStep extends React.PureComponent {
city: translate( 'City' ),
state: translate( 'State / Region / Province' ),
zip: translate( 'ZIP code' ),
Copy link

Choose a reason for hiding this comment

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

Hi! I've found a possible matching string that has already been translated 34 times:
translate( 'Zip Code' ) ES Score: 15
See 2 additional suggestions in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

@tyxla tyxla merged commit 276613f into master Feb 20, 2018
@tyxla tyxla deleted the update/jpo-business-address-add-country-field branch February 20, 2018 11:39
@oskosk
Copy link
Contributor

oskosk commented Feb 20, 2018

@AnnaMag it looked to me at first glance like if the field contents were not valid. There was a bug months ago when you reached the signup form and all of them where read and read that they were invalid (they had the clarification below the error though). I had the same feeling here, associating the red to something that I did wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants