-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat(ValidFieldPassword): add password strength meter #minor #84
Conversation
45b9a0e
to
0149d06
Compare
package.json
Outdated
@@ -30,7 +30,8 @@ | |||
"react-datetime": "^2.16.3", | |||
"react-select-async-paginate": "^0.6.1", | |||
"react-tooltip": "^3.6.1", | |||
"styled-components": "^3.2.6" | |||
"styled-components": "^3.2.6", | |||
"zxcvbn": "^4.4.2" |
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.
afaik this library size is big. is it possible to use another library?
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.
we already agree to use this lib, what is the size if I may know
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.
799kb minified and 388kb minified + gzipped
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.
@yanuaracb let me know if this is unacceptable, please see the research conclusion here https://accelbyte.atlassian.net/browse/OS-7353?focusedCommentId=150602
Thank you @rayhan-ab for raising this
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.
if it's not possible to use another library. could we lazy load this? so that it'll only be loaded if hasPasswordStrenghtMeter
is 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.
btw i found zxcvbn-ts and it's actively maintained. maybe you could take a look @aylwin-AB
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.
the size is a valid concern, please lazy load it.
zxcvbn-ts looks promising nice find @rayhan-ab , maybe worth to compare since they state it's modernized version of zxcvbn @aylwin-AB
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.
nice find @rayhan-ab, it's small too https://bundlephobia.com/package/@zxcvbn-ts/[email protected], I will try that one
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.
after try that package seems like it use the same function but break down the dictionary list, need to add this @zxcvbn-ts/[email protected] too but it's not that big
@@ -18,24 +18,31 @@ import { Button } from "../Button"; | |||
import { translation } from "../../utils/i18n"; | |||
import "../../styles/icons/fa_icons.css"; | |||
import DOMPurify from "dompurify"; | |||
import zxcvbn from "zxcvbn"; | |||
import { Enum } from "../../types"; | |||
|
|||
export interface ValidFieldPasswordProps extends Omit<ValidFieldTextProps, "type" | "rightIcon" | "isFloat"> { | |||
strengthLevelIndicator?: keyof typeof strengthLevelOrder; |
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 strengthLevelIndicator will no longer be used. please remove this.
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 still one component using this props on AP, I think we can remove it when we really do clean up and make sure we really not using it
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.
that one component is not used at all in AP, man. plus, it's only one, it should be easy to clean it up.
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 see, ok let me try to clean it up
"password.strength.level.weak": "Weak", | ||
"password.strength.level.fair": "Fair", | ||
"password.strength.level.strong": "Strong", | ||
"password.strength.level.very-strong": "Very Strong", | ||
"password.generateNew": "Generate new password", | ||
"password.poor": "Poor!", |
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 remove the old password strength translations
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.
done
const { onChange } = this.props; | ||
if (onChange) onChange(event); | ||
if (!event.target.value) return this.setState({ passwordStrenghtLevel: null }); | ||
const { score } = zxcvbn(event.target.value); |
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.
return immediately if hasPasswordStrenghtMeter
is false
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 was thinking to keep the strenght state updated so when hasPasswordStrenghtMeter
set from false to true it will get the strenght and show it. but I change it so when the hasPasswordStrenghtMeter
change from false to true it will calculate password strength.
Thank you for catching this
0149d06
to
fe09d05
Compare
seems like you update too many lines in changelog due to auto format sir. maybe you could turn it off first before saving your change in changelog |
levelColor: props.strengthLevelIndicator, | ||
}; | ||
componentDidUpdate(prevProps: ValidFieldPasswordProps) { | ||
const { hasPasswordStrenghtMeter } = this.props; |
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.
typo here. should be strength
not strenght
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.
done
{customField} | ||
</> | ||
); | ||
}; | ||
|
||
calculatePasswordStrenght = (password: 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.
typo here. should be strength
not strenght
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.
done
src/constants/password.ts
Outdated
@@ -8,4 +8,4 @@ | |||
|
|||
import { Enum } from "../types"; | |||
|
|||
export const strengthLevelOrder = Enum("poor", "weak", "average", "good", "excellent"); | |||
export const PASSWORD_STRENGTH_METER = Enum("very-weak", "weak", "fair", "strong", "very-strong"); |
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.
could we use camel case instead of kebab case so that it'll be easier to use it?
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.
done
if (onChange) onChange(event); | ||
if (!hasPasswordStrenghtMeter) return; | ||
if (!event.target.value) return this.setState({ passwordStrenghtLevel: null }); | ||
this.calculatePasswordStrenght(event.target.value); |
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 consider this issue. seems like we need to debounce calculate password strength
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 have mention this on the kick off meeting and it seems only happen on that special character being copy paste some time, it seems fine for now we can update the packages when they fix it..
also I will try the zxcvbn-ts that you mention hopefully they don't have this bug
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.
unfortunately this issue still happen at zxcvbn-ts I'm adding debounce
fe09d05
to
14264a2
Compare
i think it's more like a minor change rather than patch because even though we only update one component, the update is kinda big, involving a new library and deprecating a prop. wdyt? @aylwin-AB |
toolTipIconEye = React.createRef<HTMLElement>(); | ||
|
||
componentDidMount() { | ||
setTimeout(() => { | ||
ReactTooltip.rebuild(); | ||
}, 100); | ||
const options = { |
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.
return immediately if hasPasswordStrenghtMeter
is false?
border-radius: 4px; | ||
margin: 10px 0; | ||
.bar { | ||
background-color: $base-120; |
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.
after taking a look in my local, i think it's too dark and it's hard to distinguish the filled and unfilled bar. could we have a lighter color for the unfilled one?
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.
after checking again, i think it's different from the figma design
border-radius: 4px; | ||
margin: 10px 0; | ||
.bar { | ||
background-color: $base-120; |
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.
after checking again, i think it's different from the figma design
14264a2
to
d0386b5
Compare
d0386b5
to
e9bd68c
Compare
add password strength meter at
ValidFieldPassword
, add propshasPasswordStrenghtMeter
to show password strength meteruse package https://www.npmjs.com/package/zxcvbn, for determined the password strenght based on research on ticket https://accelbyte.atlassian.net/browse/OS-7353?focusedCommentId=150602
ticket https://accelbyte.atlassian.net/browse/OS-7368
design https://www.figma.com/file/WDZgiufhRsCbUWosWVXsDXUa/Login-Web?node-id=6527%3A4760