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
(fix) Moved all form validation logic for vitals and biometrics form into zod #1800
base: main
Are you sure you want to change the base?
Conversation
Size Change: +37 B (0%) Total Size: 11.1 MB ℹ️ View Unchanged
|
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 Vineet! A couple of remarks on this, but nice work!
], | ||
); | ||
|
||
const decimalRefinement = (key: keyof VitalsBiometricsFormData, schema: z.ZodNumber) => { |
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 this validation rule is unnecessary. Nowhere else I'm aware of do we require a field to have a decimal.
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.
We are requiring to have a decimal, we are either requiring to have no decimal or only 1 decimal place only.
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 entirely sure why we want to truncate the input precision either, but ok...
I think you can also use the expression:
value * 10 % 1 === 0
To validate that the number has exactly one decimal point. (More generally x * 10^n % 1 ==0
where x
is the value and n
is the number of decimal places to allow1).
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.
Yess, I didn't think of it this way.
Thanks a lot!!!
I implement the same!!!
packages/esm-patient-vitals-app/src/vitals-biometrics-form/vitals-biometrics-form.component.tsx
Outdated
Show resolved
Hide resolved
@@ -88,6 +89,18 @@ export function generatePlaceholder(value: string) { | |||
} | |||
} | |||
|
|||
export function getDecimalCountForField(key: keyof VitalsBiometricsFormData) { |
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 way this function works is, I think, very opaque, which makes it a bad candidate to stick in common utilities.
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 also don't love that we're hard-coding a rule here instead of, say, checking the concept's allowDecimal
result.
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.
checking the concept's allowDecimal result.
I didn't find any property by the name of allowDecimal
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.
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 a lot!!!
I looked into the response dev3.openmrs.org/openmrs/ws/rest/v1/concept?q=vital signs&v=full
, and didn't get the allowDecimal
.
Sorry for the trouble!
Thanks again!
packages/esm-patient-vitals-app/src/vitals-biometrics-form/vitals-biometrics-form.component.tsx
Outdated
Show resolved
Hide resolved
return decimalRefinement(key, numberSchema); | ||
}; | ||
|
||
const VitalsAndBiometricFormSchema = z |
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.
If possible, decimalRefinement()
, getZodNumberSchema()
and this should be refactored to be outside the component. If not, they should be wrapped in useCallback()
and useMemo()
, because as it stands, VitalsAndBiometricFormSchema
will be a different object on each render, which may cause a re-render cascade.
packages/esm-patient-vitals-app/src/vitals-biometrics-form/vitals-biometrics-form.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-vitals-app/src/vitals-biometrics-form/vitals-biometrics-form.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-vitals-app/src/vitals-biometrics-form/vitals-biometrics-form.component.tsx
Outdated
Show resolved
Hide resolved
Could you please resolve the conflicts and update this, @vasharma05. |
fee16b5
to
bf8f107
Compare
Requirements
Summary
This PR makes the following updates:
FormProvider
exported from RHF.Screenshots
Screen.Recording.2024-04-23.at.15.47.10.mov
Related Issue
None
Other