-
Notifications
You must be signed in to change notification settings - Fork 164
(feat) Add the ability to change password from edit profile form #968
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
base: master
Are you sure you want to change the base?
Conversation
| "changePassword": { | ||
| "newPassword": "Change Password", | ||
| "confirmNewPassword": "Retype new password", | ||
| "error": "<1/> {{errorMessage}}" |
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.
add a custom Component and an error message
| name="signup" | ||
| noValidate="noValidate" | ||
| onSubmit={e => editProfile(e, props, toast)} | ||
| onSubmit={props.handleSubmit} |
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.
utilizing the formik handleSubmit for better security in terms of validation
| }), | ||
| validationSchema, | ||
| handleSubmit: (values, { props }) => { | ||
| return editProfile(values, props, toast) |
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.
assigning the editProfile with the validated data
| is: (newPassword) => newPassword && newPassword.length > 0, | ||
| then: Yup.string().oneOf([Yup.ref('newPassword')], 'Passwords must match').required('Retype new password'), | ||
| otherwise: Yup.string().notRequired(), | ||
| }), |
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.
improving the schema for better error handling
| (props.errors['password'] && | ||
| <Trans t={t} i18nextKey="editProfile.inputs.changePassword.error"> | ||
| <ErrorIcon fontSize="small" /> {{errorMessage: props.errors['password']}} | ||
| </Trans> |
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 the Trans component from react-i18next to improve the field error message with Icons
|
hello @coderatomy , you did a great job here! Thanks for that! few things that I'd like us to deliberate on:
What I propose is something like this |
|
@NdibeRaymond I agree about this as well. I see that @srish approved this more simple design here (#904 (comment)). I think the rationale why we have mixed both "Edit Profile" and "Account Settings" workflow is because the Edit Profile page allows you to change username, location, personal details etc. Which is very similar to "account" related info. Hence, we can, for now also allow changing password via that workflow as well. We can have a dedicated change password/account settings button some where which can do a similar workflow as yours and then we can phase out this change. We would need UX input for that and I think it would look similar to your approach |
| color: 'var(--primary-color2)', | ||
| }, | ||
| fieldHelperTextStyle: { | ||
| '&.MuiFormHelperText-root.Mui-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.
Try sending the classnames to mui component rather than using their generated class names. it would hassle when upgrading libraries as they change how the classnames (or styles) (they usually support it) are created or update dom structure
<ErrorComponent classNames={{
errorComponent: classNames.myErrorClassName
}} />
| <InputLabel | ||
| className={classes.customLabelStyle} | ||
| htmlFor="password" | ||
| <Grid container style={{ display: 'block'}}> |
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 are you changing the grid system to block? its just gonno affect responsiveness
FYI If you aare changing to block system just use div instead
| endAdornment={ | ||
| <InputAdornment position="end"> | ||
| <IconButton | ||
| aria-label="toggle password visibility" |
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.
labels need translations
| username: Yup.string().required('required'), | ||
| user_location: Yup.string().min(1, 'min').required('required'), | ||
| password: Yup.string().required('required'), | ||
| password: Yup.string().required('Your password is required'), |
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 errors needs translation
|
hey @coderatomy please link this PR to #984 so that redundant work can be avoided. Once this PR is merged I can work on moving the edit profile to the sidenav. Thanks! |
|
hey @aqsaaqeel , i think you can go on with this, just go through the comments here to get the context, then get out what might be useful from this PR and use it. My main priority now is on the Migrations. Kindly help me with that. |
Alright! |
Summary
fixes #904
Adds ability to change password from
/edit-profileChanges
handleSubmitfunction from formik such that form submission is immediately cancelled if any validation errors exist. Sometimes our custom validation can have some loop holes.Screencasts
Screencast.from.2023-10-26.15-27-09.webm