Skip to content
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

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

aylwin-AB
Copy link
Collaborator

@aylwin-AB aylwin-AB commented Aug 31, 2022

add password strength meter at ValidFieldPassword, add props hasPasswordStrenghtMeter to show password strength meter

use 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

image

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"
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator

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

Copy link
Collaborator Author

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

Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator Author

@aylwin-AB aylwin-AB Aug 31, 2022

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

Copy link
Collaborator Author

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;
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

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.

Copy link
Collaborator Author

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!",
Copy link
Collaborator

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

Copy link
Collaborator Author

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);
Copy link
Collaborator

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

Copy link
Collaborator Author

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

@aylwin-AB aylwin-AB force-pushed the OS-7368-fe-ui-libap-implement-password-s branch from 0149d06 to fe09d05 Compare August 31, 2022 05:21
@rayhan-ab
Copy link
Collaborator

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;
Copy link
Collaborator

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

Copy link
Collaborator Author

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) => {
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -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");
Copy link
Collaborator

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?

Copy link
Collaborator Author

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);
Copy link
Collaborator

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

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

@aylwin-AB aylwin-AB force-pushed the OS-7368-fe-ui-libap-implement-password-s branch from fe09d05 to 14264a2 Compare August 31, 2022 09:26
@rayhan-ab
Copy link
Collaborator

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 = {
Copy link
Collaborator

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;
Copy link
Collaborator

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?

Copy link
Collaborator

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;
Copy link
Collaborator

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

@aylwin-AB aylwin-AB force-pushed the OS-7368-fe-ui-libap-implement-password-s branch from 14264a2 to d0386b5 Compare September 1, 2022 01:39
@rayhan-ab rayhan-ab force-pushed the OS-7368-fe-ui-libap-implement-password-s branch from d0386b5 to e9bd68c Compare September 1, 2022 01:50
@rayhan-ab rayhan-ab changed the title feat(ValidFieldPassword): add password strength meter #patch feat(ValidFieldPassword): add password strength meter #minor Sep 1, 2022
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.

4 participants