-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add password strength meter #1949
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
5093f76
to
348cd03
Compare
a386786
to
d91a8b5
Compare
5158e2b
to
d7f1d41
Compare
d7f1d41
to
1ae6f1d
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.
Thank you, Oleg - great addition to the password checks 🔥
Please check comments before merging
); | ||
}; | ||
|
||
type UsePasswordValidationProps = { |
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.
nit, optional: maybe move UsePasswordValidationProps
& usePasswordValidation
hook to the top of the file?
if (validate) { | ||
const validateResult = validate(val); | ||
|
||
if (checkPasswordStrength && validateResult === true) { |
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.
validate?: (val: string) => string | boolean;
Is string result for the errors?
validateResult
can be a string? won't validateResult === true
return true as well?
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.
validateResult could be either a sting or boolean
A non-empty string is a truthy value, but is not strictly equal to true
minLength = MIN_LENGTH, | ||
validate, | ||
...rest | ||
}: PasswordInputProps<T, U>) => { | ||
const { register } = useFormContext<T>(); | ||
const [showPassword, setShowPassword] = useState<boolean>(false); | ||
const { validatePassword, PasswordStrengthBar } = usePasswordValidation({ |
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.
nit: maybe validatePasswordStrength
to avoid confusion with validate function from the input
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.
Please apply comments from desktop version of PasswordInput
as well
const colors = [colorScheme, "red.500", "yellow.500", "green.500"]; | ||
const passwordError = form.formState.errors[inputName]; | ||
|
||
const getColor = (index: number) => { |
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.
nit: maybe getSectionColor
& sectionIndex
?
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.
@OKendigelyan please consider renaming / adding a comment - it's a bit hard to understand what the function from a first glance
|
||
type PasswordStrengthBarProps = { | ||
score: number; | ||
colorScheme: string; |
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.
nit: maybe defaultSectionColor
or defaultColor
here?
16efe96
to
e83a7cb
Compare
b6d5638
to
8c422c1
Compare
@@ -55,6 +57,7 @@ export const ChangePasswordMenu = () => { | |||
data-testid="new-password-confirmation" | |||
inputName="newPasswordConfirmation" | |||
label="Confirm password" | |||
minLength={0} |
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.
shall we make it the default for the password input so that we don't need to repeat it everywhere?
switch (score) { | ||
case 1: | ||
case 2: | ||
return "Weak"; |
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.
capitalized strength looks strange in UI tbh, maybe use the lowercased one?
ccaa9e0
to
8b6874d
Compare
8b6874d
to
240337f
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 for the changes @OKendigelyan
Approved with some minor comments 🚀
const colors = [colorScheme, "red.500", "yellow.500", "green.500"]; | ||
const passwordError = form.formState.errors[inputName]; | ||
|
||
const getColor = (index: number) => { |
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.
@OKendigelyan please consider renaming / adding a comment - it's a bit hard to understand what the function from a first glance
}; | ||
|
||
type UsePasswordValidationProps = { | ||
color?: string; |
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.
maybe defaultBarColor or something similar? Not very obvious what this param is for with the current name
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.
I think color
would be more concise in this case, given that the only place where color can be used as a prop in this hook is the PasswordStrengthBar, so I don't think it can be too confusing
const [showPassword, setShowPassword] = useState<boolean>(false); | ||
const { validatePasswordStrength, PasswordStrengthBar } = usePasswordValidation({ | ||
inputName, | ||
minLength, |
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.
Why MIN_LENGTH is defined here and not in usePasswordValidation?
Could it have values other than 12?
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.
Is it to use it here for the error message?
Maybe instead we can keep everything related to the validation in usePasswordValidation
, and export the minLength from there?
const color = useColor(); | ||
|
||
const error = errors[inputName]; | ||
const errorMessage = error?.message as string; | ||
|
||
const handleValidate = (val: string) => { | ||
if (validate) { | ||
const validateResult = validate(val); |
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.
nit: maybe validationResult?
240337f
to
4ef32d7
Compare
4ef32d7
to
9ff1492
Compare
9ff1492
to
84be53f
Compare
Proposed changes
Task link
Types of changes
Steps to reproduce
Screenshots
Web
Screen.Recording.2024-09-25.at.15.19.18.mov
Desktop
Screen.Recording.2024-09-25.at.15.20.37.mov
Checklist