-
Notifications
You must be signed in to change notification settings - Fork 3
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
525 implement ability to change password #527
525 implement ability to change password #527
Conversation
re_new_password: formData.reNewPassword | ||
}) | ||
.then(() => toast.success('Пароль успішно змінено')) | ||
.catch(() => toast.error('Виникла помилка. Можливо, вказано невірний поточний пароль')); |
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 .catch()
block only displays a generic error message. Consider improving error handling by parsing the error response from the server to provide more specific feedback to the user
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! Initially, I was outputting the error status code, but the team advised against it as it's not user-friendly. I'll consider better ways to inform the user.
useEffect(() => { | ||
props.currentFormNameHandler(props.curForm); | ||
}, [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.
useEffect
has [props]
as a dependency, which could lead to unnecessary re-executions if props
object itself changes but not its values. Consider destructuring props
outside useEffect
and using specific properties in the dependency array
e.g.
const { currentFormNameHandler, curForm } = props;
useEffect(() => {
currentFormNameHandler(curForm);
}, [currentFormNameHandler, curForm]);
label="Поточний пароль" | ||
updateHandler={handleInputChange} | ||
value={formData.currentPassword} | ||
requredField={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.
There seems to be a typo in the requredField
prop (should be requiredField
). Moreover, HTML5 form validation could be utilized by using the required attribute on inputs instead of handling this manually
const handleInputChange = (e) => { | ||
e.preventDefault(); | ||
const updatedFormData = { ...formData, [e.target.name]: e.target.value }; | ||
setFormData(updatedFormData); | ||
(updatedFormData.newPassword !== '' && | ||
!PASSWORD_PATTERN.test(updatedFormData.newPassword)) ? | ||
setInvalidPasswordError('Пароль не відповідає вимогам') : | ||
setInvalidPasswordError(null); | ||
(updatedFormData.reNewPassword !== '' && | ||
updatedFormData.newPassword !== updatedFormData.reNewPassword) ? | ||
setPasswordsDontMatchError('Паролі не співпадають') : | ||
setPasswordsDontMatchError(null); | ||
}; |
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 function updates state multiple times which can lead to performance issues or unexpected renders. It would be better to consolidate state updates into a single function call if possible
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.
const handleInputChange = (e) => {
e.preventDefault();
const { name, value } = e.target;
const newFormData = { ...formData, [name]: value };
const newInvalidPasswordError = newFormData.newPassword !== '' && !PASSWORD_PATTERN.test(newFormData.newPassword)
? 'Пароль не відповідає вимогам'
: null;
const newPasswordsDontMatchError = newFormData.reNewPassword !== '' && newFormData.newPassword !== newFormData.reNewPassword
? 'Паролі не співпадають'
: null;
// Update all states in one go
setFormData(newFormData);
setInvalidPasswordError(newInvalidPasswordError);
setPasswordsDontMatchError(newPasswordsDontMatchError);
};
return ( | ||
<div className={css['password-field__item']}> | ||
<div className={css['password-field__label-wrapper']}> | ||
{props.requredField && |
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.
requredField
seems to be misspelled. The correct spelling should be requiredField
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.
Yes, you're right! Initially, I was reusing the existing HalfFormField component in the project where this typo was present (the component was taking prop 'requredField' instead of 'requiredField'), and then I forgot to correct the improper prop passing when I wrote my PasswordField.
<span | ||
className={css['password-visibility']} | ||
onClick={togglePassword} | ||
> |
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 toggle
button for showing/hiding the password doesn’t have an accessible label or an aria-label
attribute. Adding accessible labels should be useful for users who rely on screen readers
e.g.
<span className={css['password-visibility']} onClick={togglePassword} aria-label={showPassword ? "Hide password" : "Show password"}>
{!showPassword ? <EyeInvisible /> : <EyeVisible />}
</span>
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, will be done.
{!showPassword ? <EyeInvisible /> : <EyeVisible />} | ||
</span> | ||
</div> | ||
{(props.requredField || props.error) && |
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 error message section will display even if props.error
is an empty string and props.requredField
is true. It might be more logical to display this section only if there is an error message to show
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, I'll consider it.
required={props.requredField} | ||
/> | ||
</div> | ||
<span |
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.
Using a <span>
for the password visibility toggle is not ideal because spans are not inherently focusable or interactive elements. Consider using a <button>
instead, which is more appropriate for actions and improves accessibility
e.g.
<button className={css['password-visibility']} onClick={togglePassword} type="button">
{!showPassword ? <EyeInvisible /> : <EyeVisible />}
</button>
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 agree with this point. I added a span there because it's done that way in our LoginPage component.
…tructuring for useEffect dependency array
Added ability to change password for user.