-
Notifications
You must be signed in to change notification settings - Fork 177
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: enhanced registration form validation #81
feat: enhanced registration form validation #81
Conversation
Add zod validation for registration password and use zod-express-middleware for request validation
🦋 Changeset detectedLatest commit: 3ef254c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/app/src/AuthPage.tsx
Outdated
|
||
const onSubmit: SubmitHandler<FormData> = async data => { | ||
try { | ||
const response = await fetch(`${API_SERVER_URL}/register/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.
would be nice if we can wrap the api call using react query (check out api.ts
file)
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.
missed that, my bad
updated now!
packages/app/src/AuthPage.tsx
Outdated
|
||
if (Array.isArray(jsonData) && jsonData[0]?.errors?.issues) { | ||
return jsonData[0].errors.issues.forEach((issue: any) => { | ||
setError(issue.path[0], { |
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.
👍
type="password" | ||
className="border-0" | ||
{...form.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.
nit: not super critical. I think it would be nice to add 'confirm password' field and the icon to toggle 'show 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.
there's #40 which adds the toggle, perhaps I shouldn't duplicate it
I'm down to add the password confirmation field, but not sure if I get to it today or tomorrow, feel free to go ahead and merge it, and I can implement it sometime this week with a follow-up PR. This will also give you some time figure out if you proceed with #40 or not. How does that sound?
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.
sounds good. let me merge the pr and we can implement these features later. thanks
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.
LGTM. Great stuff. Thanks for making the changes!
Origin: #81 (comment) Add a password confirmation field to the registration form. - API will require `confirmPassword` field on the registration endpoint - The `confirmPassword` and `password` fields will be validated by zod on API validation - UI on the registration form will have a "Confirm Password" field - Validation errors will be displayed on the UI the same way other validation errors - API test covers mismatch between `confirmPassword` and `password` (simple check, can be improved)
Close #44
/register/password
API validation with zod and zod-express-middleware.AuthPage
will keep using original/login/password
API with redirect, forregister
action a request will be made insteadThings I considered but decided to omit:
/login/password
to drop redirect: might be a good idea, but not in the context of this issue