-
Notifications
You must be signed in to change notification settings - Fork 699
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
Updated NewPasswordPage.vue with password visibility toggle #12878
base: develop
Are you sure you want to change the base?
Conversation
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.
Thanks for the contribution. There are a couple things that must be addressed to ensure proper translation and accessibility.
I think that we may want to run this by our designers as well before merging, although the approach you've taken here feels nice to me.
Before merge:
- Could you update your PR to include some screenshots of the change so that our Designers can give it a look
- Implement translated messages
- Add aria-label to added KButton
- Get OK from our design team
@click="togglePassword" | ||
data-test="togglePasswordVisibility" | ||
> | ||
{{ showPassword ? 'Hide' : 'Show' }} Password |
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.
In order to be sure that the message here is translated for all users, you'd need to add two new messages in the $trs
property of the component, one each for "Hide password" and "Show password". Then you can conditionalize which message to show by referencing the keys of the objects you add to $trs
in a call to $tr(<the key of the message object you made>)
.
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.
You could pass the KButton's label to the :text
prop instead of to the default slot as you have here. It may be cleaner to do this because we also need to put the same text on the KButton's aria-label
attribute as well
Thank you for your valuable suggestions, will definitely work on it
…On Tue, 3 Dec 2024, 6:01 am Jacob Pierce, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thanks for the contribution. There are a couple things that must be
addressed to ensure proper translation and accessibility.
I think that we may want to run this by our designers as well before
merging, although the approach you've taken here feels nice to me.
Before merge:
- Could you update your PR to include some screenshots of the change
so that our Designers can give it a look
- Implement translated messages
- Add aria-label to added KButton
- Get OK from our design team
------------------------------
In
kolibri/plugins/user_auth/assets/src/views/SignInPage/NewPasswordPage.vue
<#12878 (comment)>
:
> />
+
+ <!-- Button to toggle password visibility -->
+ <KButton
+ appearance="basic-link"
+ style="margin-top: 8px; text-align: left;"
+ @click="togglePassword"
+ data-test="togglePasswordVisibility"
+ >
+ {{ showPassword ? 'Hide' : 'Show' }} Password
In order to be sure that the message here is translated for all users,
you'd need to add two new messages in the $trs property of the component,
one each for "Hide password" and "Show password". Then you can
conditionalize which message to show by referencing the keys of the objects
you add to $trs in a call to $tr(<the key of the message object you made>)
.
------------------------------
In
kolibri/plugins/user_auth/assets/src/views/SignInPage/NewPasswordPage.vue
<#12878 (comment)>
:
> />
+
+ <!-- Button to toggle password visibility -->
+ <KButton
+ appearance="basic-link"
+ style="margin-top: 8px; text-align: left;"
+ @click="togglePassword"
+ data-test="togglePasswordVisibility"
+ >
+ {{ showPassword ? 'Hide' : 'Show' }} Password
You could pass the KButton's label to the :text prop instead of to the
default slot as you have here. It may be cleaner to do this because we also
need to put the same text on the KButton's aria-label attribute as well
—
Reply to this email directly, view it on GitHub
<#12878 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BEH2ALXANFORGGTLVDXXT3T2DT3VZAVCNFSM6AAAAABSL6FCCGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINZUGIYDEMRUGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Hi @Dhanushcdivakar, thanks for your contribution and @nucleogenesis for all the guidance and review. We appreciate this work @Dhanushcdivakar. For next time, I'd like to suggest you check out the contributing guidelines and follow the assignment process described there. For new ideas, it'd be best to open a new issue at first since we need confirm the expected behavior before opening it for contribution. Thanks a lot and looking forward to further collaboration. |
Thank you for your feedback.
…On Wed, 4 Dec 2024, 9:27 pm Michaela Robosova, ***@***.***> wrote:
Hi @Dhanushcdivakar <https://github.com/Dhanushcdivakar>, thanks for your
contribution and @nucleogenesis <https://github.com/nucleogenesis> for
all the guidance and review.
We appreciate this work @Dhanushcdivakar
<https://github.com/Dhanushcdivakar>. For next time, I'd like to suggest
you check out the contributing guidelines
<https://github.com/learningequality/kolibri/blob/develop/CONTRIBUTING.md>
and follow the assignment process described there. For new ideas, it'd be
best to open a new issue at first since we need confirm the expected
behavior before opening it for contribution. Thanks a lot and looking
forward to further collaboration.
—
Reply to this email directly, view it on GitHub
<#12878 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BEH2ALR7RLZXFTASN4VOZF32D4Q6LAVCNFSM6AAAAABSL6FCCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJXHA2TQNBSGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This pull request introduces a feature to toggle password visibility on the NewPasswordPage.vue component. A "Show/Hide Password" button has been added to allow users to toggle between displaying the password in plain text or keeping it hidden for better user experience during password creation.
Changes:
Added a showPassword data property to manage the visibility state of the password.
Implemented a "Show/Hide Password" button that toggles the visibility of the password field.
Updated the PasswordTextbox component to dynamically change the input type (text or password) based on the visibility state.
Ensured that the existing form validation and password update functionality remain unchanged.
Manual Verification:
Verified that clicking the "Show Password" button reveals the password in plain text.
Verified that clicking the "Hide Password" button hides the password again.
Ensured that the password validation and submission processes still work as intended.