Skip to content

Password strength gauge stimulus #3243

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
merged 2 commits into from
Apr 2, 2018

Conversation

yeraydiazdiaz
Copy link
Contributor

This builds on #3219 addressing #3197 and #3223

More importantly includes a refactor of the password gauge functionality to Stimulus.

There's a few decisions I made around the ability to compose components using JS "mixins" which I'm not sure it's "idiomatic Stimulus" if there is such a thing. Here's three captures of the end results in three areas:

  • Reset password:
    reset_password

  • Register:
    register

  • Update password:
    update_password

Hopefully it makes sense and we can move forwards with Stimulus using this pattern.

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.

Although it's probably too soon to call something "idiomatic stimulus", I think it would be more idiomatic to make these separate controllers than controllers that inherit from each other. There's not much to share between them and I think it would generally be cleaner.

That said, I have not given this PR a super close look, so if there's a good reason why that's wrong I could probably be convinced.

@@ -11,10 +11,18 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/* global zxcvbn */

Copy link
Member

Choose a reason for hiding this comment

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

Accidental comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eslint appeaser 😄

@di
Copy link
Member

di commented Mar 14, 2018

Also, thank you for tackling this!

@yeraydiazdiaz yeraydiazdiaz force-pushed the password-strength-gauge-stimulus branch 2 times, most recently from 8d4e01a to 73b6ec1 Compare March 24, 2018 18:03
@yeraydiazdiaz yeraydiazdiaz force-pushed the password-strength-gauge-stimulus branch from 73b6ec1 to f570e07 Compare March 24, 2018 18:11
@yeraydiazdiaz
Copy link
Contributor Author

I've updated this to a much simpler approach of adding multiple controllers to the form.

I feel like some lines are getting a big too wide to read comfortably. I'd be in favor of something like:

{{ form.password_confirm(
    placeholder="Confirm password",
    required="required",
    class_="form-group__input",
    data_target="password.password password-match.passwordMatch",
    data_action="keyup->password-match#checkPasswordsMatch") }}

Would we consider disabling the template linting rule around single line echo statements to allow that?

@di
Copy link
Member

di commented Mar 24, 2018

@yeraydiazdiaz I think that'd be fine.

@yeraydiazdiaz
Copy link
Contributor Author

Seems it's not something we can disable in the html_lint command line 😞

@di di merged commit 5d13b14 into pypi:master Apr 2, 2018
nitinprakash96 pushed a commit to nitinprakash96/warehouse that referenced this pull request Apr 3, 2018
@yeraydiazdiaz yeraydiazdiaz deleted the password-strength-gauge-stimulus branch April 3, 2018 08:30
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.

2 participants