Skip to content

Add password strength gauge to recover password and account settings #3128

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

Merged

Conversation

yeraydiazdiaz
Copy link
Contributor

As suggested by @nlhkabu

I couldn't quite test it on the recover password page due to a KeyError: 'token.password.secret' when trying to request a password reset, I'm possibly missing something. I did verify it works on the account settings and on the register form.

I also notice we're starting to use Stimulus and I'd love to try it out, would've probably been a good fit for the password gauge. Not sure if it's something worth reimplementing at this stage given it's working already.

@di
Copy link
Member

di commented Mar 3, 2018

@yeraydiazdiaz Looks like your local dev environment is missing these new variables which were recently added. It doesn't pull them in automatically when they're changed, so you'll need to do a docker-compose build web (I think you don't need to remove the current container first) to get it working again.

My plan for all the frontend JS is to eventually convert it to using Stimulus. I agree the password reset gauge would be a good fit, so if you're interested in porting it to a Stimulus controller, I think it's definitely worth doing! (In a separate PR though please 🙂)

@di
Copy link
Member

di commented Mar 3, 2018

Also, writing a jinja2 macro in the base HTML template to remove some of the duplication that is happening in this PR is probably a good idea.

@@ -219,7 +219,7 @@ <h2 class="sub-title">Change Password</h2>
</div>
<div class="form-group">
<label class="form-group__label" for="name">New Password</label>
{{ change_password_form.new_password(placeholder="Select a new password", required="required", class_="form-group__input") }}
{{ add_password_strength(change_password_form.new_password(placeholder="Select a new password", required="required", class_="form-group__input")) }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we need to pass the field into the macro like this? I was imagining this being something like:

<label class="form-group__label" for="name">New Password</label>
{{ change_password_form.new_password(placeholder="Select a new password", required="required", class_="form-group__input") }}
{{ field_errors(change_password_form.new_password) }}
{{ password_strength_gauge() }}

(Also I think the errors should probably come before the gauge)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input field needs to be wrapped in a div tag to properly place the tooltips since they're implemented on pseudo-elements. Since it's not immediately obvious I was trying to avoid having to manually do it on each template but it just makes things less explicit, fixed now.

@di di merged commit 7a8df4d into pypi:master Mar 6, 2018
@di
Copy link
Member

di commented Mar 6, 2018

Thanks @yeraydiazdiaz!

@nlhkabu
Copy link
Contributor

nlhkabu commented Mar 7, 2018

Just saw this @yeraydiazdiaz - thank you!!!

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.

3 participants