-
Notifications
You must be signed in to change notification settings - Fork 1k
Disable form and show warning on mismatch when updating password #3219
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
Conversation
9fac705
to
3c149fb
Compare
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.
We should enable this for the other password forms as well.
import PasswordController from "./password_controller"; | ||
|
||
export default class extends PasswordController { | ||
static targets = PasswordController.targets.concat(["newPassword", "confirmPassword", "passwordsMatchMessage", "submit"]); |
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.
Let's just call this matchMessage
.
|
||
import PasswordController from "./password_controller"; | ||
|
||
export default class extends PasswordController { |
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 instead of extending the PasswordController
, we can just make this it's own controller. data-controller
can take multiple controller names, e.g. data-controller="password password-match"
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 didn't know you could specify more than one Stimulus controller in data-controller
, that makes composing components in Javascript completely unnecessary, makes me like Stimulus even more
this.submitTarget.setAttribute("disabled", ""); | ||
} else { | ||
this.passwordsMatchMessageTarget.classList.remove("hidden"); | ||
if (this.newPasswordTarget.value === this.confirmPasswordTarget.value) { |
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.
Instead of just checking these two targets, we probably want to simplify this to make sure all passwordMatchTargets
have the same non-empty value. That way we can reuse this on other pages just by applying the target to whichever password fields we want to match.
3c149fb
to
30e7727
Compare
@di I've updated the PR with your suggested changes and also included it on the register and reset password forms.
|
Fixes #3197