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

Trims trailing spaces from email address #1378

Merged

Conversation

gutofoletto
Copy link
Contributor

@gutofoletto gutofoletto commented Feb 8, 2019

Changes the onEmailChange method to trim spaces from the email address on SignIn form as requested on opencollective/opencollective#1701. Also, added a cypress test to verify the 'feature'.

This is to prevent users from getting validation errors when copying and pasting from password managers

If there is anything that could be improved, let me know :D

@vercel
Copy link

vercel bot commented Feb 8, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

cy.visit('/signin');
cy.get('input[name=email]').type(' user@opencollective.com ');
cy.get('button[type=submit]').should('not.be.disabled');
});
Copy link

Choose a reason for hiding this comment

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

What about test the trim, changing the field and check if has no spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@vercel vercel bot temporarily deployed to staging February 8, 2019 15:33 Inactive
@znarf
Copy link
Member

znarf commented Feb 8, 2019

@gutofoletto Thank you so much looks great. I re-triggered CI because it was failing, let's see.

@znarf znarf requested review from znarf and Betree February 8, 2019 21:04
@Betree
Copy link
Member

Betree commented Feb 10, 2019

Why not handling the trim at the lowest level - in the SignIn component? There are places in the code where we may want to use the SignIn component outside of the SignInOrJoinFree.
It would also make it testable on our styleguide page.

Otherwise the feature looks good to me, it works as intended. We should be aware that users with an email like "my email address"@website.com (it's a valid address) will have troubles to input their addresses but they can still do it and most websites actually consider this as invalid (such users are looking for troubles anyway).

Thanks for writing cypress tests here, much appreciated!

@gutofoletto
Copy link
Contributor Author

@Betree I just thought it would be better to transform the string in the same place the state is being set. But I could easily move the trim function to the SignIn component and trim target.value. It makes sense to have that logic encapsulated inside the component.

Don't know if I got it right. But, yeah...that would just trim the spaces from both ends. I don't know if an email like "my email address@provider.com" should be valid though. Perhaps we could use a regex instead of the trim() function. What do you think?

Thanks for the feedback :D

@Betree
Copy link
Member

Betree commented Feb 11, 2019

@gutofoletto I like the idea of trimming the target.value. Trim is great, there's no perfect Regex for emails anyway so the simpler the better :)

Unfortunatelly "my email address"@provider.com is indeed valid though providers like GMail probably don't allow it:

space and special characters "(),:;<>@[] are allowed with restrictions (they are only allowed inside a quoted string, as described in the paragraph below, and in addition, a backslash or double-quote must be preceded by a backslash);

But with the fix you've made they could still add it by playing a little bit with the input, so I'm good with the solution you proposed. Again, such users would be looking for troubles anyway 😉

@znarf
Copy link
Member

znarf commented Feb 14, 2019

@gutofoletto Could you please rebase the branch against master? It should fix the circleci failure.

Sorry for that! 🙏

@gutofoletto
Copy link
Contributor Author

Sure! No problem! I will do that ASAP ;)

changes the onEmailChange method to trim spaces from the email address on signIn form. This is to
prevent users from getting validation errors when copying and pasting from password managers
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.

None yet

4 participants