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

Delete Site: toLowerCase() before validating domain #7458

Merged
merged 1 commit into from
Aug 17, 2016

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Aug 14, 2016

At WordCamp Boston, a user came up to me while I was working the Jetpack booth and asked me to walk her through the process of deleting her site. So, I did that. But, as I walked the user through how to delete her site on her iPad, I realized that we weren't being very friendly to mobile users in our validation logic.

Specifically, our logic checks that the domain the user entered is exactly the same as the site's acutal domain. The problem with this is that the mobile keyboard by default capitalizes the first character. So, the user, after having entered her site domain multiple times became frustrated.

This PR aims to fix this by lower casing the entered domain before running validation logic as well as turning off autoCapitalize.

To test:

  • Checkout update/delete-site-to-lower branch
  • Go to /settings/general/$site where $site is a WP.com site
  • Click "Delete site" at bottom of page
  • Click "Delete site" button at bottom of page
  • Fill out domain in modal
  • Ensure delete button is NOT disabled
  • Follow same steps on mobile

Test live: https://calypso.live/?branch=update/delete-site-to-lower

@ebinnion ebinnion self-assigned this Aug 14, 2016
@ebinnion ebinnion force-pushed the update/delete-site-to-lower branch 3 times, most recently from 1d0258b to c7f3b05 Compare August 15, 2016 15:51
@ebinnion ebinnion 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 Aug 15, 2016
@@ -195,7 +198,12 @@ export const DeleteSite = React.createClass( {
}
} )
}</p>
<input className="delete-site__confirm-input" type="text" valueLink={ this.linkState( 'confirmDomain' ) }/>
<input
Copy link
Member

Choose a reason for hiding this comment

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

I'd personally indent the input and its attributes one more tab (although the misformatting was not introduced in this PR).

@tyxla
Copy link
Member

tyxla commented Aug 15, 2016

That's a nice catch! Also, PR looks and works fine for me 👍

I've commented inline with a very minor nitpick.

Other than that, I think it is good to go :shipit:

@tyxla tyxla added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 16, 2016
@ebinnion ebinnion force-pushed the update/delete-site-to-lower branch from c7f3b05 to ffe5eae Compare August 17, 2016 22:05
@ebinnion ebinnion force-pushed the update/delete-site-to-lower branch from ffe5eae to 33a2822 Compare August 17, 2016 22:06
@ebinnion ebinnion merged commit 003eb11 into master Aug 17, 2016
@ebinnion ebinnion deleted the update/delete-site-to-lower branch August 17, 2016 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Settings All other general site settings. Sites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants