-
-
Notifications
You must be signed in to change notification settings - Fork 85
Present password feedback directly to user #4484
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
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
|
zxcvbn has some i18n stuff in it I think, I'll have a look. |
|
Sorry @matthewbadeau I completely forgot to look at this again. Having reminded myself, zxcvbn has various language packages in it, so all I should need to do is enable the ones other than english (that tbh I should have already put there). I don't think that needs to be in this PR, so if you're happy with this, it's good to go I think, and we can do the other languages separately. |
|
@matthewbadeau are you ready for this to be merged in? It's still marked as draft, but I'm happy to merge it in as-is. |
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.
There are accessibility issues in these changes.
| <tfoot> | ||
| <tr> | ||
| <td align="center" size="13px" colspan="7"> | ||
| <img src="https://raw.githubusercontent.com/all-contributors/all-contributors-cli/1b8533af435da9854653492b1327a23a4dbd0a10/assets/logo-small.svg"> |
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.
This image is missing a text alternative. This is a problem for people using screen readers.
|
@Floppy Small technical difficulty as accidentally merged all commits from July 13th into this branch. I need a moment to fix it. But yes, once I've fixed that, it should be ready |
9c50699 to
ef47994
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.
👏 You fixed the issue(s)! Great work.
|
@Floppy Fixed my git mistakes 😅 I'll mark this one ready |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Checklist
🚨 Please review the guidelines for contributing to this repository. 🚨
Warnings
Summary
This presents zxcvbn's feedback directly to the user in the password input screen. The side-effect being that it will render a progress bar between 0-25%. Right now the progress bar does not render unless it's an increment of (100/4). The progress bar looks like a plain grey box until 25% and users are, understandably, not aware that the password they're inputting needs to be more complex.
I'm not sure if this is the best approach. Ideally, we should have a width of 1 as the minimum value and then increase from there but zxcvbn's scores are only 0-4, so we can't get a middle progress value.
Also, the strings aren't translated. It's presenting the string from zxcvbn directly, so I'm not sure how to override that with translatable strings.
Linked issues
Partially addresses #4483
Description of changes