Skip to content
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

Add form validations #105

Merged
merged 20 commits into from
Apr 26, 2020
Merged

Add form validations #105

merged 20 commits into from
Apr 26, 2020

Conversation

gauravchl
Copy link
Member

@gauravchl gauravchl commented Apr 22, 2020

Summary:

  • This PR implements the form validation using React Hook form and JOI validation schema

  • First commit cf24584 implements the basic structure and reusable components required for validation.

  • Second commit 6f4ae37 is an example of how to apply validation to any form.

    1. Create a validation schema(eg: SignUpForm.schema.js)
    2. Import schema into form and pass it down.
    3. Feel free to update validation rules in schema currently applied to signup form.
  • Currently validation is only applied to signup form and server side error messages specific to individual field is not implemented yet.

Related Issue:

TODO:

  • Write tests for validation

Copy link
Member

@lpatmo lpatmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, thank you so much for working on this, @gauravchl! :)

Forgive me if this isn't yet ready for review; there were two issues I noticed:

1/ When I start typing into the form, I see this error:

image

2/ On submit, I'm seeing this error:
image

package.json Show resolved Hide resolved
src/components/Auth/AuthForm.spec.js Outdated Show resolved Hide resolved
src/components/Auth/SignUpForm.js Outdated Show resolved Hide resolved
src/components/Auth/SignUpForm.schema.js Show resolved Hide resolved
src/components/Auth/SignUpForm.js Show resolved Hide resolved
@lpatmo
Copy link
Member

lpatmo commented Apr 23, 2020

Also cc'ing @peoray here to help review, since you were originally assigned to #93!

@lpatmo lpatmo requested a review from peoray April 23, 2020 00:07
@gauravchl
Copy link
Member Author

2/ On submit, I'm seeing this error:
image

@lpatmo Are you getting this error after axios call?

Copy link
Contributor

@angelocordon-omada angelocordon-omada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good direction @gauravchl - I think we should also try to add specs here testing the expected validation behaviors.

src/components/Auth/AuthForm.spec.js Outdated Show resolved Hide resolved
src/components/form/index.js Outdated Show resolved Hide resolved
src/components/form/index.js Outdated Show resolved Hide resolved
@lpatmo
Copy link
Member

lpatmo commented Apr 24, 2020

2/ On submit, I'm seeing this error:
image

@lpatmo Are you getting this error after axios call?

Yeah, I was getting that error after submit! It looks like if you make this change, it works:

   email: Joi.string()
-    .email({ tlds: { allow: false } })
+    .email({ tlds: { allow: true } })

Not sure why, though -- makes me think tlds is broken in https://github.com/hapijs/joi/blob/master/API.md.

UPDATE: Actually never mind, I'm confused because it's broken again, but after I restarted both my backend server and frontend app and I changed it back to false it worked??

Copy link
Member

@lpatmo lpatmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your time on this, Gaurav! :D This is looking awesome! Will let @angelocordon review as well, and afterwards I think this should be ready to merge...

Copy link
Contributor

@sebbel sebbel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a few comments, good job @gauravchl :)

src/components/Auth/SignUpForm.js Outdated Show resolved Hide resolved
src/components/Auth/SignUpForm.js Show resolved Hide resolved
src/components/Auth/SignUpForm.js Show resolved Hide resolved
src/components/Auth/SignUpForm.js Show resolved Hide resolved
src/components/Auth/SignUpForm.schema.js Show resolved Hide resolved
src/components/form/Field.js Outdated Show resolved Hide resolved
);
};

Field.propTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are any of these required?

src/components/Auth/SignUpForm.schema.js Outdated Show resolved Hide resolved
src/components/form/Form.js Outdated Show resolved Hide resolved
src/components/form/Form.js Show resolved Hide resolved
@angelocordon
Copy link
Contributor

I think we can do a final pass and merge it in soon. Thanks!

Copy link
Contributor

@angelocordon angelocordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for a few clean up (see comments) and validation testing.

@angelocordon angelocordon linked an issue Apr 26, 2020 that may be closed by this pull request
@angelocordon angelocordon added this to the v3.0.1 milestone Apr 26, 2020
@angelocordon angelocordon removed the request for review from peoray April 26, 2020 04:33
Copy link
Contributor

@angelocordon angelocordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Form Validations to AuthForms
5 participants