562: reset password feature#771
Conversation
| display: flex; | ||
| justify-content: center; | ||
| `; | ||
|
|
||
| interface LoginFormProps { |
There was a problem hiding this comment.
refactored LoginForm to put styles in one shared file
| </PageLayout> | ||
| <LoginLayout heading={t("dashboard.login.login")}> | ||
| <LoginController /> | ||
| </LoginLayout> | ||
| ); |
There was a problem hiding this comment.
refactored Login to use a shared layout as I use this also for /forgotten-password and /reset-password pages
|
|
||
| interface ResetPasswordData { | ||
| newPassword: string; | ||
| confirmPassword: string; |
There was a problem hiding this comment.
confirmPassword not needed in the be (it does nothing). Still wanted another password value so that the user could essentially double-check what they're changing their password to.
This is so in the form it triggers a fe error if newPassword and confirmPassword do not match
| onChange: ({ value }) => (!value ? t("dashboard.login.emailMissing") : undefined), | ||
| onChangeAsyncDebounceMs: 500, | ||
| onChangeAsync: async ({ value }) => { | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); |
There was a problem hiding this comment.
Why do we need this onChangeAsync with synthetic wait?
IT feels like it's easier to add to onChange, or just use some email validator
There was a problem hiding this comment.
good point, I just copied the email field from the Login without questioning this so have tidied it up
| }, | ||
| }); | ||
| const schema = createResetPasswordSchema(t); | ||
| console.log("s", schema); |
There was a problem hiding this comment.
Probably better to remove
| name="confirmPassword" | ||
| validators={{ | ||
| onChange: ({ value, fieldApi }) => { | ||
| const resultValue = schema.shape.newPassword.safeParse(value); |
There was a problem hiding this comment.
About all validation in this file:
I think it's better to try form level validation
https://tanstack.com/form/v1/docs/framework/react/guides/validation
As far as i understand it's the docs for a newer version, but still worth a try to do it like that.
Otherwise I would propose to wrap it in a function. It will remove code duplication with other validators, and will make reading this file easier. For confrimPassword matching with newPassword, I feel, if form level validation wont work, it's easier and more readable to make a check in place
There was a problem hiding this comment.
I see what you mean and good suggestion. I find it easier to stick to the zod Schema pattern particularly as we're using it for other validators for user-defined inputs (e.g. on the Profile edits).
I've wrapped the validation logic in one function and it does look a lot cleaner
There was a problem hiding this comment.
I meant using form level validation with zod. Based on that I believe, it's probably possible to do something like that
const form = useForm({
defaultValues: {
newPassword: "",
confirmPassword: "",
token: urlTokenParam || "",
},
onSubmit: async ({ value }) => {
resetPassword(value);
},
validators: {
onChange: ({ value }) => {
const result = schema.safeParse(value);
if (!result.success) {
const errors = toErrors(result.error.issues);
// where toErrors transforms zod format to { 'key1': 'errormessage1', 'key2': 'errormessage2', ...etc }
return {
fields: errors,
}
}
},
},
});There is even some package for tanstack/react-form that does that. But it's sus, it's not in their main branch
It also would solve the issue with current code I just thought of: if user typed in a password, then matching confirmPassword, then changed the password, confirmPassword field will not raise an error. I actually didnt run your code and tried, but it feels like it will behave like that
I'm totally okay leaving validation per field. But it's worth checking and fixing this "changing password after you typed in proper confirmPassword" thing
| confirmPassword: z.string().min(1, { message: t("dashboard.login.passwordMissing") }), | ||
| }) | ||
| .refine((data) => data.newPassword === data.confirmPassword, { | ||
| message: "dashboard.login.passwordNotMatching", |
There was a problem hiding this comment.
Why not t("dashboard.login.passwordNotMatching") here? You would avoid all this startsWith('.dashboard') unpleasantness from component
There was a problem hiding this comment.
ye good point, have change this and removed the startsWith('.dashboard') from the component
| textColor={form.state.canSubmit && !isPending ? "var(--color-white)" : "var(--color-grey-400)"} | ||
| textHoverColor="var(--color-magnolia)" | ||
| disabled={ | ||
| !form.state.canSubmit || !form.state.values.newPassword || !form.state.values.confirmPassword || isPending |
There was a problem hiding this comment.
I'm almost sure, you can use form.state.isValid instead newPassword and confirmPassword checks. Maybe even omit isValid, since there is a chance it's included in canSubmit
There was a problem hiding this comment.
very true, it would seem that this is indeed covered by the canSubmit so have simplified it to that
|
thanks @kiborrdis ! Have implemented the changes |
Description
Adds a feature to reset password which follows these steps:
/login/forgotten-passwordwhere they are prompted to enter their email/reset-password?token=[token]/loginRelated Issues
Closes #562
Changes
How to manually test
belocal devfego to/loginand click "forgot password?". Ensure you're sending data tobelocalhostbeyou'll see the email link/reset-password?token=[token]and paste it into thefeso that you have:localhost:3000/en/reset-password?token=[token]/loginwhere you can test your new password which should workScreenshots / Demos
/forgotten-password
On success
/reset-password?token=[token]
Checklist