Fixed issue #342 -- Add hide/unhide button for passwords in change password#352
Fixed issue #342 -- Add hide/unhide button for passwords in change password#352Alien501 wants to merge 5 commits intodjangoindia:mainfrom
Conversation
…n change password
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to Django India! 🎉🇮🇳
We're so happy to see you contribute. Your efforts mean a lot to us and we're excited to have you on this journey.
Before we proceed, please take a moment to review our contribution guide. It's packed with all the information you need to make your contribution seamless.
If you're fixing an issue from the Django India issue tracker, remember to get it assigned to you before you acutally start working on it.
If you ever need help or just want to chat, the Django India discord server is always open. We're here for you!
And while you're here, don't forget to ⭐ star our repo to show your support! It helps the community grow.
Thank you for being a part of Django India. Together, let's make an amazing community! 🚀
DevilsAutumn
left a comment
DevilsAutumn
left a comment
There was a problem hiding this comment.
Thanks for this patch @Alien501 ! Left comments.
A small suggestion: Please get the issue assigned to you before you start working on it. This prevents rework if multiple people are working on the same issue.
|
|
There was a problem hiding this comment.
What do you want me to do here exactly?
There was a problem hiding this comment.
the space was removed in this line. add that space back so that there if no diff in this line.
DevilsAutumn
left a comment
DevilsAutumn
left a comment
There was a problem hiding this comment.
Looks good to me, Thanks ! @Alien501
DevilsAutumn
changed the title
| </FormControl> | ||
| <Button | ||
| className='absolute right-1 top-1/2 -translate-y-1/4 bg-transparent text-foreground hover:bg-transparent mt-2 w-max' | ||
| onClick={(e) => {e.preventDefault(); togglePasswordVisibility(field.name)}}> |
There was a problem hiding this comment.
Do we really need to preventDefault here ?
There was a problem hiding this comment.
Yeah ig, since it is nested inside a form there is chance of it to be considered to be a submit action when clicked on it. Can be replaced with type="button"
|
@CodeRabbit fullreview |
|
@Yadavanurag13 I'll conduct a full review of this PR that adds the hide/unhide toggle functionality for password fields in the change password section. ✅ Actions performedFull review triggered. |
WalkthroughAdds password visibility toggles to the ChangePasswordForm component by introducing local state, toggle handler, eye/eye-slash icons, and switching input types between password and text for new and confirm password fields. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Form as ChangePasswordForm
participant UI as Password Inputs
User->>Form: Click "eye" on New/Confirm field
activate Form
Note right of Form: togglePasswordVisibility(field)
Form->>Form: Flip showPassword[field] boolean
deactivate Form
Form->>UI: Re-render with input type = text/password
UI-->>User: Field text visibility updates (eye/eye-slash icon toggles)
%% Error/edge cases can be visual (no backend calls involved)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| <Button | ||
| className='absolute right-1 top-1/2 -translate-y-1/4 bg-transparent text-foreground hover:bg-transparent mt-2 w-max' | ||
| onClick={(e) => {e.preventDefault(); togglePasswordVisibility(field.name)}}> | ||
| {field.name === 'newPassword' | ||
| ? showPassword.newPassword | ||
| ? <FaEyeSlash size={18} /> | ||
| : <FaEye size={18} /> | ||
| : showPassword.confirmPassword | ||
| ? <FaEyeSlash size={18} /> | ||
| : <FaEye size={18} /> | ||
| } | ||
| </Button> |
There was a problem hiding this comment.
Add accessible name/state to the visibility toggle button.
The icon-only button currently has no accessible name or pressed state, so screen-reader users will encounter an unlabeled control and cannot tell whether the password is visible. Please surface an accessible label that reflects the action (show/hide) and expose the toggle state via aria-pressed. Once you make it a plain type="button", the extra preventDefault handler is no longer necessary.
Apply this diff to address the issue:
- <Button
- className='absolute right-1 top-1/2 -translate-y-1/4 bg-transparent text-foreground hover:bg-transparent mt-2 w-max'
- onClick={(e) => {e.preventDefault(); togglePasswordVisibility(field.name)}}>
+ <Button
+ type="button"
+ aria-label={
+ field.name === 'newPassword'
+ ? (showPassword.newPassword ? 'Hide new password' : 'Show new password')
+ : (showPassword.confirmPassword ? 'Hide confirm password' : 'Show confirm password')
+ }
+ aria-pressed={
+ field.name === 'newPassword'
+ ? showPassword.newPassword
+ : showPassword.confirmPassword
+ }
+ className='absolute right-1 top-1/2 -translate-y-1/4 bg-transparent text-foreground hover:bg-transparent mt-2 w-max'
+ onClick={() => togglePasswordVisibility(field.name)}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| className='absolute right-1 top-1/2 -translate-y-1/4 bg-transparent text-foreground hover:bg-transparent mt-2 w-max' | |
| onClick={(e) => {e.preventDefault(); togglePasswordVisibility(field.name)}}> | |
| {field.name === 'newPassword' | |
| ? showPassword.newPassword | |
| ? <FaEyeSlash size={18} /> | |
| : <FaEye size={18} /> | |
| : showPassword.confirmPassword | |
| ? <FaEyeSlash size={18} /> | |
| : <FaEye size={18} /> | |
| } | |
| </Button> | |
| <Button | |
| type="button" | |
| aria-label={ | |
| field.name === 'newPassword' | |
| ? (showPassword.newPassword ? 'Hide new password' : 'Show new password') | |
| : (showPassword.confirmPassword ? 'Hide confirm password' : 'Show confirm password') | |
| } | |
| aria-pressed={ | |
| field.name === 'newPassword' | |
| ? showPassword.newPassword | |
| : showPassword.confirmPassword | |
| } | |
| className='absolute right-1 top-1/2 -translate-y-1/4 bg-transparent text-foreground hover:bg-transparent mt-2 w-max' | |
| onClick={() => togglePasswordVisibility(field.name)}> | |
| {field.name === 'newPassword' | |
| ? showPassword.newPassword | |
| ? <FaEyeSlash size={18} /> | |
| : <FaEye size={18} /> | |
| : showPassword.confirmPassword | |
| ? <FaEyeSlash size={18} /> | |
| : <FaEye size={18} /> | |
| } | |
| </Button> |
🤖 Prompt for AI Agents
In frontend/src/containers/Users/ChangePasswordForm.tsx around lines 125 to 136,
the icon-only visibility toggle button lacks an accessible name and state;
change the Button to type="button", remove the unnecessary e.preventDefault()
call, add a dynamic aria-label that reflects the action ("Show password" or
"Hide password") based on the current visibility for the given field, and set
aria-pressed to a boolean representing whether the password is currently visible
(true when visible, false when hidden) so screen readers receive both label and
toggle state.
4 participants
Closes #342
Added an enhancement feature in profile page. Added toggle button to show/hide password.
Changes
Type of change
Demo
Summary by CodeRabbit