-
Notifications
You must be signed in to change notification settings - Fork 14
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
Form Field Validation #85
Conversation
56eba91
to
898d2ef
Compare
898d2ef
to
f50acdc
Compare
@@ -2,12 +2,12 @@ import React, { useContext } from 'react'; | |||
|
|||
import Wizard from 'components/common/Wizard'; | |||
import FormUtils from 'util/FormsUtils.js'; | |||
import formValidation from 'utils/formValidation'; |
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.
Maybe this stuff moves to core FormUtils
?
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.
Looks good to @ceruleancee and I.
Only one thing we noticed (not a big deal right now): The API Key and API Secret accept values with just spaces. Should it be doing a not blank check there (require a value with non-space chars)? We can also just take care of this later.
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.
Looks good to me.
One thing I did notice was that the the "setting up your CloudWatch Log Group" and "Pipeline Rules" links open in the same tab, and it may be easier on the users to open in a new tab rather than start the process over.
Provides help message and validation to each field.
Could add a list of errors at the top of the form as well, but this should work for the time being.
Feel free to share anything that may already exist for field validation.