-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: add password visibility button to authentication pages #572
Conversation
✅ Deploy Preview for seium-stg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great opportunity to create a PasswordInput component. There's a lot of duplicated code running around, and the extra useStates everywhere just to show/hide the eye are really cluttery.
You could even use the regular Input to create the PasswordInput, although I think it would be best to separate the two. That way you could also remove the right children from the Input, which would literally never be used for anything other than the eye icon on a password input.
@tiago-bacelar @ruilopesm , I agree with you that creating a new component is a good idea. You mentioned not using the Input component, so should I create a new independent component, right? In that case, I'll need to duplicate some code, mainly the styles. Is that not a problem? I understand the idea of removing the 'right' prop, and I think it's better to have the styles repeated than that right prop. However, is there a way to avoid repeating the styles? Because if, for some reason, somebody changes the Input styles, it would also be necessary to edit them in PasswordInput, and this can be forgotten. (by now I will create the component from the right prop, to already replace everything with the component, then I edit consonant your feedback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 🙌🏼
I already fix the requested changes about the id/name prop. I added the id/name prop again, and fix it from ̶A̶l̶s̶o̶,̶ ̶t̶o̶ ̶d̶e̶l̶e̶t̶e̶ ̶t̶h̶e̶ ̶r̶i̶g̶h̶t̶ ̶p̶r̶o̶p̶ ̶f̶r̶o̶m̶ ̶t̶h̶e̶ ̶I̶n̶p̶u̶t̶ ̶c̶o̶m̶p̶o̶n̶e̶n̶t̶,̶ ̶I̶ ̶c̶r̶e̶a̶t̶e̶d̶ ̶t̶h̶e̶ ̶P̶a̶s̶s̶w̶o̶r̶d̶I̶n̶p̶u̶t̶ ̶w̶i̶t̶h̶o̶u̶t̶ ̶a̶n̶y̶ ̶d̶e̶p̶e̶n̶d̶e̶n̶c̶y̶ ̶o̶n̶ ̶t̶h̶e̶ ̶I̶n̶p̶u̶t̶,̶ ̶b̶u̶t̶ ̶t̶o̶ ̶a̶v̶o̶i̶d̶ ̶r̶e̶p̶e̶a̶t̶e̶d̶ ̶s̶t̶y̶l̶e̶s̶,̶ ̶t̶h̶a̶t̶ ̶c̶o̶u̶l̶d̶ ̶c̶a̶u̶s̶e̶ ̶d̶i̶f̶f̶e̶r̶e̶n̶t̶ ̶s̶t̶y̶l̶e̶s̶,̶ ̶a̶f̶t̶e̶r̶ ̶a̶ ̶m̶o̶d̶i̶f̶i̶c̶a̶t̶i̶o̶n̶ ̶a̶t̶ ̶o̶n̶l̶y̶ ̶o̶n̶e̶ ̶o̶f̶ ̶t̶h̶e̶ ̶c̶o̶m̶p̶o̶n̶e̶n̶t̶,̶ ̶I̶ ̶e̶x̶p̶o̶r̶t̶e̶d̶ ̶t̶h̶e̶ ̶I̶n̶p̶u̶t̶ ̶s̶t̶y̶l̶e̶s̶ ̶f̶r̶o̶m̶ ̶t̶h̶e̶ ̶I̶n̶p̶u̶t̶ ̶c̶o̶m̶p̶o̶n̶e̶n̶t̶.̶ ̶W̶h̶a̶t̶ ̶d̶o̶ ̶y̶o̶u̶ ̶t̶h̶i̶n̶k̶ ̶o̶f̶ ̶t̶h̶e̶ ̶r̶e̶s̶u̶l̶t̶?̶ ̶(̶I̶ ̶k̶n̶o̶w̶ ̶t̶h̶a̶t̶ ̶i̶s̶n̶'̶t̶ ̶t̶h̶e̶ ̶m̶o̶s̶t̶ ̶c̶o̶m̶m̶o̶n̶ ̶t̶h̶i̶n̶g̶,̶ ̶s̶o̶ ̶I̶'̶m̶ ̶w̶a̶i̶t̶i̶n̶g̶ ̶f̶e̶e̶d̶b̶a̶c̶k̶)̶ |
This reverts commit 3ac9ec6.
To prevent repeated styles across the project were created a InputBase wrapper to be used internally when creating Input components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! 💪
Added see Password Button at all login/signup forms.
I didn't checked the changes at
pages/register/[uuid].js
(I'm wating for feedback in how access this pageseems to be unused and maybe will be deleted).I also needed edit Input Component to accept a
right children
component.Issue related: #527