-
Notifications
You must be signed in to change notification settings - Fork 215
Change password form in Settings - Account #5089
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
Change password form in Settings - Account #5089
Conversation
You are of course correct, I got confused by the similar-looking issue names 😅 I fixed the mistake. |
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.
Ahoj Jakube, Kubo, hi Jakub 🙂!
This is very well done - thanks a lot for following all the guidance, and also for taking time to examine and describe the differences you observed in such a detail. Much appreciated.
I would say that experience is better now because for example we can see "Password should be at least 8 characters long" until it's really fixed 👍 But leaving my opinions aside, most importantly, as you too noted, this is aligned precisely with how KDS and similar forms work in both Kolibri and Studio, so it's okay.
I've too tested in my local Studio and haven't observed any issues. Code makes sense. Just left one minor note for tests, but it's not blocking.
contentcuration/contentcuration/frontend/settings/pages/__tests__/changePasswordForm.spec.js
Outdated
Show resolved
Hide resolved
Remove dependencies on internal implementation.
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 a lot @Jakoma02, very nice contribution.
Ahoj Míšo! 🙂 🇨🇿 Thank you for the kind feedback and for cleaning up the tests. I am looking forward to collaborating in the future! |
The "Change password" form was updated to use KDS components instead of Vuetify components. The
PasswordField
component was replaced withKTextBox
, andVForm
was replaced with aform
. Validation was implemented usingformMixin
. The tests were modified to pass with the new validation interface, and a new test case was added to improve clarity (the previous description of the test said that it tested if the validation failed when the passwords did not match, but in fact, it failed because the password was too short).I tested the changes by interacting with the form in a browser and checking the validation behavior in various scenarios, with the interface set to both LTR and RTL languages. I also tested the mobile experience by using the "Toggle device" developer feature in Chromium.
References
Solves #5061.
Reviewer guidance
The behavior of the old and new forms differs in several aspects outlined below. The differences stem from technical differences between the two approaches. Special care should probably be taken to ensure that the new behavior is expected and acceptable.
When a text field is edited to have invalid content, the validation error immediately appears. In contrast, in the original form, validation errors are not displayed when the text field has focus, and are only shown when it stops being focused. The new behavior matches the behavior of other forms that are already using KDS (e.g., the Request more space form). If the original behavior is desired,
KTextbox
should probably be changed to reflect this expectation.When the new password is cleared to be an empty string, the validation error "Password should be at least 8 characters long" is displayed. In contrast, in the original form, the validation error "This field is required" is displayed. This is caused by
formMixin
only returning the binary result of the validation instead of the error message. If this is undesirable,formMixin
should probably be refactored to be slightly more versatile in returning more than two validation results.When both the new password and confirmation fields are edited and then cleared, the confirmation field shows no validation error. In contrast, in the original form, the validation error "This field is required" is displayed for the confirmation field. This is caused by the
required
property being ignored byformMixin
when a custom validator is also specified, and is connected to the previous point.