-
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
Yup validation for SHAS intake form #74
Yup validation for SHAS intake form #74
Conversation
65d14a6
to
6d286aa
Compare
6ff69f5
to
d690a42
Compare
For me there is some odd validation behaviour.
|
On top of Sharolyn's comment
|
firstName: stringRequired.label('First name'), | ||
lastName: stringRequired.label('Last name'), | ||
phoneNumber: stringRequired.label('Phone number'), | ||
email: stringRequired.label('Email'), |
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.
Should be validating against a email regex
export const Regex = Object.freeze({
// https://emailregex.com/
// HTML5 - Modified to require domain of at least 2 characters
EMAIL: '^[a-zA-Z0-9.!#$%&’*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\\.[a-zA-Z0-9-]{2,})+$'
});
|
||
// Form validation schema | ||
const YesNoUnsureSchema = string().required().oneOf(YesNoUnsure); | ||
const prevYesSelected = (prevQuestion: string) => prevQuestion === BASIC_RESPONSES.YES; |
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'm not feeling the necessity of pulling this little bit out into it's own function?
basic: object({ | ||
isDevelopedByCompanyOrOrg: string().required().oneOf(YesNo).label('Project developed'), | ||
isDevelopedInBC: string().when('isDevelopedByCompanyOrOrg', { | ||
is: (previousQuestion: string) => previousQuestion === BASIC_RESPONSES.YES, |
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.
This naming threw me off for a bit. May be better to just stick to calling it value
as there is no true context tie to the previous form value outside of the current layout. The actual value coming in is whatever is defined in the when
.
location: object({ | ||
projectLocation: string().required().label('Location'), | ||
streetAddress: string().when('projectLocation', { | ||
is: (prevQuestion: string) => prevQuestion === ProjectLocation[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.
Should import the constants in this file and check directly against them. If someone were to inadvertently flip the array values around the validation would explode.
}), | ||
appliedPermits: array().of( | ||
object({ | ||
permitTypeId: number().required().max(255).label('Permit type'), |
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.
Should not have max
here. While it's entirely unlikely the permit type IDs will go beyond this it could happen.
04eeecc
to
0da0f72
Compare
Alert banner says "one of more..." It should say "one or more..." Everything else looks good to me! Thanks!! |
0da0f72
to
36b3004
Compare
document.getElementById('form')?.scrollIntoView({ behavior: 'smooth' }); | ||
} | ||
|
||
function value(data: any) { |
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.
Unclear function name. Not sure why it was renamed from confirmSubmit
@@ -161,9 +171,23 @@ onBeforeMount(async () => { | |||
:click-callback="clickCallback" | |||
title="Basic info" | |||
icon="fa-user" | |||
:class="{ | |||
'p-red': |
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 p-
prefix is a primevue directive. We should not be using it in our own code. app-error-color
or something would be a better fit, and added into our main.scss
@@ -302,9 +326,19 @@ onBeforeMount(async () => { | |||
:click-callback="clickCallback" | |||
title="Housing" | |||
icon="fa-house" | |||
:class="{ 'p-red': validationErrors.includes('housing') }" |
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.
Magic string. Should be referencing INTAKE_FORM_CATEGORIES. Same css class name comment as above.
@@ -900,9 +955,23 @@ onBeforeMount(async () => { | |||
:click-callback="clickCallback" | |||
title="Permits & Reports" | |||
icon="fa-file" | |||
:class="{ | |||
'p-red': |
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 p-
prefix is a primevue directive. We should not be using it in our own code. app-error-color
or something would be a better fit, and added into our main.scss
const YesNoUnsureSchema = string().required().oneOf(YesNoUnsure); | ||
const stringRequired = string().required().max(255); |
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.
Naming styles should be similar. Consider StringRequiredSchema
?
36b3004
to
fa17281
Compare
d568020
to
fa17281
Compare
…eForm Update PrimeVue to 3.52.0 Rebase to latest ShasIntakeForm.vue changes Integrate multiple WIP intake form validation rules Additional validation error message handling
fa17281
to
98ddbf2
Compare
Description
Added frontend validation to new intake form.
Some minor UI adjustments (new error messages, margin/padding) to validation error messages
Types of changes
New feature (non-breaking change which adds functionality)
Checklist
Further comments
PR to existing feature/intake-form branch