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

Adjust password strength checker copy #1452

Merged
merged 1 commit into from
May 23, 2017

Conversation

el-mapache
Copy link
Contributor

Why: The new copy reads better!

Screenshot:

screen shot 2017-05-23 at 1 50 19 pm

@el-mapache
Copy link
Contributor Author

I'm not sure if its worth it to write an integration test for this...maybe if we go through and audit all the copy at some point?

Copy link
Member

@hursey013 hursey013 left a comment

Choose a reason for hiding this comment

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

There is one string Add another word or two. Uncommon words are better. that includes a period at the end already - I think it makes sense to remove it since it will be added if gets concatenated with another string?

@el-mapache
Copy link
Contributor Author

el-mapache commented May 23, 2017

Right you are, removed!

Copy link
Member

@hursey013 hursey013 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!

@hursey013
Copy link
Member

hursey013 commented May 23, 2017

Oops, spoke too soon, looks like that's one of the few strings that we have hardcoded into a test.

@el-mapache
Copy link
Contributor Author

Ugh yes. Just updated!

@hursey013
Copy link
Member

Good to go.

@el-mapache el-mapache force-pushed the ab-change-password-strength-copy branch from f8c67c3 to 4407c74 Compare May 23, 2017 20:27
**Why**: The new copy reads better!
@el-mapache el-mapache merged commit 309818e into master May 23, 2017
@el-mapache el-mapache deleted the ab-change-password-strength-copy branch May 23, 2017 20:41
@esgoodman
Copy link
Contributor

LGTM.

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.

3 participants