-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
lib: Improve password fields #19220
lib: Improve password fields #19220
Conversation
Currently we these 4 levels of messages for passwords "Weak password" < "Good password" < "Strong password" < "Excellent password". |
dc8bf4d
to
71e7c81
Compare
needsdesign → moving to draft |
The contrast between the text and its background is not high enough. The color of the text should be the darker gold. You can see this in PF demos:
Here, you can see how PF uses brighter versions for icons, progress bars, etc. and the darker related color for text:
Colors are specified here:
This isn't just a problem with the warning colors either, but it's most noticeable with it, as it has the least contrast. The other colors need to use the text variants too. |
Yes. 👍 |
71e7c81
to
8fd6ef9
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.
It's a lot better. Thanks!
However, the meter (progress bar) doesn't update based on the strength.
Screencast.from.2023-09-20.09-59-43.webm
01d286c
to
03c3b35
Compare
Fixed the issue @garrett Screencast.from.2023-10-17.15-06-03.webm |
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 was waiting on the password meter fix. Thanks! Approved.
(But please move that line of CSS.)
.ct-password-strength-meter { | ||
grid-gap: var(--pf-v5-global--spacer--xs); | ||
|
||
.pf-v5-c-progress__description, .pf-v5-c-progress__status { | ||
display: none; | ||
} | ||
|
||
inline-size: var(--pf-v5-global--spacer--2xl); | ||
} |
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.
.ct-password-strength-meter { | |
grid-gap: var(--pf-v5-global--spacer--xs); | |
.pf-v5-c-progress__description, .pf-v5-c-progress__status { | |
display: none; | |
} | |
inline-size: var(--pf-v5-global--spacer--2xl); | |
} | |
.ct-password-strength-meter { | |
grid-gap: var(--pf-v5-global--spacer--xs); | |
inline-size: var(--pf-v5-global--spacer--2xl); | |
.pf-v5-c-progress__description, .pf-v5-c-progress__status { | |
display: none; | |
} | |
} |
Parent CSS should be grouped together at the top. (This doesn't change anything, but it's better when it's not scattered.)
d54860f
to
68b4e8f
Compare
Ready for code-review |
68b4e8f
to
1bae20a
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.
1bae20a
to
c175744
Compare
That looks much saner -- most tests are okay, just the accounts tests have the updated pixels for the reveal buttons. I'll push these. |
c175744
to
1ade659
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.
Thanks!
@@ -77,20 +82,34 @@ export const PasswordFormFields = ({ | |||
setPasswordMessage(strength.message); | |||
}); | |||
} else { | |||
setPasswordStrength(""); | |||
setPasswordStrength(); |
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 added line is not executed by any test. Details
Fixes #19199
Screencast.from.2023-08-22.14-30-21.webm