Skip to content
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

Disable form and show warning on mismatch when updating password #3219

Merged
merged 3 commits into from
Mar 19, 2018

Conversation

yeraydiazdiaz
Copy link
Contributor

Fixes #3197

@lgh2
Copy link
Contributor

lgh2 commented Mar 12, 2018

I tested it and it looks like it works:

passwords-not-match
pw-match

Copy link
Member

@di di left a 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"]);
Copy link
Member

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 {
Copy link
Member

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"

Copy link
Contributor Author

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) {
Copy link
Member

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.

@di
Copy link
Member

di commented Mar 14, 2018

Looks like there is a small bug where it thinks the passwords match even though they are both empty:

mar-14-2018 13-07-28

@nlhkabu nlhkabu self-assigned this Mar 16, 2018
@yeraydiazdiaz
Copy link
Contributor Author

@di I've updated the PR with your suggested changes and also included it on the register and reset password forms.

  • Update password:

update_password

  • Register:

register

  • Reset password:

reset_password

@di di merged commit 2ccc096 into pypi:master Mar 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants