-
Notifications
You must be signed in to change notification settings - Fork 1
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
Adds validation to fix email addresses. #1075
Conversation
This has a similar issue that fix commitment info tool has with the state and formik. Not 100% sure the best way to implement formik in this instance. I opted to just add to the mapped emails. The only real issue I found with this way is that the confirm button isn’t blocked when an illformed address is entered. |
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.
The only real issue I found with this way is that the confirm button isn’t blocked when an illformed address is entered.
I saw a few times where typing an invalid email address and clicking Confirm sent the invalid email in the GraphQL mutation. But it appears that the server ignored it without returning an error. It's a little weird but probably OK.
It looks like we have a form inside of a form now. I'm not sure if that's bad or not, but if it needs to be changed, we can do that when we research performance. (EDIT: this is not true, I didn't read the code carefully).
onSubmit={async (values) => { | ||
await handleChange(id, index, values.newEmail); |
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.
handleChange
doesn't return a promise.
onSubmit={async (values) => { | |
await handleChange(id, index, values.newEmail); | |
onSubmit={(values) => { | |
handleChange(id, index, values.newEmail); |
error={true} | ||
data-testid="statusSelectError" | ||
> | ||
{errors.newEmail && errors.newEmail} |
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.
{errors.newEmail && errors.newEmail} | |
{errors.newEmail} |
Preview branch generated at https://MPDX-8162-fix-email-address-validation.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against 67614f0 No significant changes found |
Nested forms are not supported in HTML technically. |
@wrandall22 Good to know. I'll rework this. |
@canac I'm not seeing the nested form, am I missing something? I initially thought you were referring to the EmailValidationForm but it looks like that falls outside of the other form. |
You're right, I wasn't reading carefully. I thought I have no issues with the code you have here. Sorry for slowing this down! |
@canac All good, thanks for looking it over! |
Description
Checklist: