-
Notifications
You must be signed in to change notification settings - Fork 0
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
FS-5018: remove html5 validation and add email validation #135
Conversation
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 also please do a test. with an existing fund
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.
Hey @NarenderRajuB thanks for this, have tested locally by:
- Going to https://form-designer.levellingup.gov.localhost:3000/app/designer/f7luKiZDuI
- Adding a new page linked from the first page (which contains an email address field) to allow the first page to be submitted
- Previewing the first page
- Entering some invalid email address and submitting form
Behaviour was as expected 👍
However, I'm struggling to review the code a bit, so maybe you could help me out. My understanding was that the requirement here was effectively to take the EmailAddressField
from the digital-form-builder
base repo, copy paste it into Adapter and make some small tweak to add the desired validation.
Based on this, I'd expect a minimal diff between the base repo class and this one, and a clear explanation of what validation we're performing and how this has been implemented by the new code. This would make this PR easy to review.
What I see instead are a number of changes between the two classes e.g.,:
Base repo EmailAddressField
:
getFormSchemaKeys() {
return {
[this.name]: this.formSchema,
};
}
This new implementation:
getFormSchemaKeys() {
return getFormSchemaKeys(this.name, "string", this);
}
Likewise there are other differences that I'm not sure about. So can I ask that you describe the approach you took here and explain any differences between the EmailAddressField
in the base repo and the new implementation? Thanks
ab3d748
to
6a24aa9
Compare
|
@wjrm500 i did followed the approach we are using for websitefield.ts |
ab1c074
to
6bac789
Compare
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.
I think fair change so approving
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.
Thanks for making the changes @NarenderRajuB
Add optional
6bac789
to
a22e32d
Compare
|
https://mhclgdigital.atlassian.net/browse/FS-5018
Change description
Remove HTML5 validation and add email address validation in backend