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

Signup: Fix username suggestions not working #10795

Merged
merged 3 commits into from
Jan 23, 2017

Conversation

bisko
Copy link
Contributor

@bisko bisko commented Jan 19, 2017

Username suggestions were broken due to a prop name change in #10599.

This PR improves the fetching of the suggested username to use Redux and properly just pass the prop to SignupForm

To test:

Test free username:

  1. Start Signup
  2. Go through Signup
  3. Select a domain name which would result a free username suggestion ( e.g. something123455123 )
  4. Choose the free wordpress.com address
  5. Reach the User step
  6. See if the username field is properly filled up with the domain slug you wrote.

To test a taken username:

  1. Start Signup
  2. Go through Signup
  3. Type kwight in the domain search box
  4. Choose kwight.me
  5. Choose a random plan
  6. Reach the User step
  7. See if the username is kwightweb or something different than kwight

Todo:

  • The unit tests are broken for the PR. Will work on fixing them.

cc @kwight :)

@bisko bisko added [Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. [Status] In Progress labels Jan 19, 2017
@bisko bisko self-assigned this Jan 19, 2017
@bisko bisko requested review from michaeldcain and tyxla January 19, 2017 20:39
@matticbot
Copy link
Contributor

Username suggestions were broken due to a prop name change in #10599.

This PR improves the fetching of the suggested username to use Redux and properly just pass the prop to `SignupForm`
@bisko bisko force-pushed the fix/signup-fix-suggested-username-not-appearing branch from df4db84 to 8f56175 Compare January 20, 2017 17:09
@bisko bisko added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jan 20, 2017
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Works as described 👍

Code also looks good, I've just posted a couple of non-blocking nitpicks that you can feel free to skip and leave for another PR if you wish.

/**
* External dependencies
*/
import get from 'lodash/get';
Copy link
Member

Choose a reason for hiding this comment

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

Here we can use the single member import syntax, which is the recommended one recently:

import { get } from 'lodash';

( state ) => ( {
suggestedUsername: getSuggestedUsername( state )
} ),
( dispatch ) => ( {
Copy link
Member

Choose a reason for hiding this comment

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

Dispatch will be mapped to action creators automatically, so:

( dispatch ) => ( {
	recordTrackEvent: ( event, data ) => {
		dispatch( recordTracksEvent( event, data ) );
	}
} )

can be shortened to just:

{ recordTracksEvent }

@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 23, 2017
Copy link
Member

@michaeldcain michaeldcain left a comment

Choose a reason for hiding this comment

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

LGTM

@bisko bisko merged commit 7e518cf into master Jan 23, 2017
@bisko bisko deleted the fix/signup-fix-suggested-username-not-appearing branch January 23, 2017 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants