-
Notifications
You must be signed in to change notification settings - Fork 0
T14: Build the reset password form #22
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: main
Are you sure you want to change the base?
Conversation
…e will be processed once backend method for reset password is created.
justinpchen94
left a comment
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.
small nits mostly
| setPasswordError(passwordError); | ||
|
|
||
| // TODO: Once handler in backend is created come back here to process the response | ||
| const response = await fetch('/api/auth/reset-password', { |
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.
in general it's nice to put all the API calls into a separate file esp since API calls eventually will be reused. this react component is made slightly messier by having to also include details about the API request itself here, such as the headers and etc
| const ResetPassword: React.FC = () => { | ||
| const [newPassword, setNewPassword] = useState<string>(''); | ||
| const [confirmNewPassword, setConfirmNewPassword] = useState<string>(''); | ||
| const [passwordError, setPasswordError] = useState<string | null>(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.
nit: passwordValidationError slightly more helpful since this one is currently broad and the other error is more specific
| value={confirmNewPassword} | ||
| onChange={handleConfirmNewPasswordChange} | ||
| /> | ||
| {passwordMatchError && ( |
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.
not blocking: not the worst thing in the world but given that your UI only has 2 fields, it's slightly confusing to have the errors shown in 2 different places. i know you're doing a different design than the wireframes but notice that there are differences between input level and global level errors, and the input level errors are shown directly contextually under the inputs where the issues are.

Description:
UI with filled inputs:

Password criteria error being thrown:

Password matching error being thrown:
