Fix numeric validator #1382
Fix numeric validator #1382
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
No new version will be released. Current: v3.20.8 [DataDrivenFormsBot] |
Codecov Report
@@ Coverage Diff @@
## master #1382 +/- ##
=======================================
Coverage 95.09% 95.09%
=======================================
Files 210 210
Lines 3647 3647
Branches 1271 1271
=======================================
Hits 3468 3468
Misses 179 179
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
jspannu919
force-pushed
the
fix_numeric_validator
branch
from
4b55ffb to
5ddca06
Compare
There was a problem hiding this comment.
@jspannu919 Thanks for the contribution. I have one small suggestion.
| @@ -90,7 +90,7 @@ export const numericality = memoize( | |||
| lessOrEqual = selectNum(lessOrEqual, lessThanOrEqualTo); | |||
|
|
|||
| return prepare((value) => { | |||
| if (!value) { | |||
| if (value == null) { | |||
There was a problem hiding this comment.
I think we should cover all "falsy" values
| if (value == null) { | |
| if (typeof value !== 'number' && typeof value !== 'string') { |
There was a problem hiding this comment.
The next block to this is checking if the value is a valid number or not, handling these here will lead to no proper message being displayed.
There was a problem hiding this comment.
You are correct. My point is that we also have to consider undefined in addition to null. I guess we can let the rest of the types through this check. Oh and also, can you make sure to use the strict equality ===?
There was a problem hiding this comment.
I have not added strict equality intentionally, to cover "null" as well as "undefined" case.
There was a problem hiding this comment.
These strings will be handled by the isNumber check below as you pointed out. We like to use the strict equality operator whenever possible to avoid unexpected "truthy" results. It's a matter of convention.
We can go ahead and merge/release the PR afterward.
There was a problem hiding this comment.
Sorry if I was not able to explain it properly. I didn't meant to cover the null and undefined "strings" but the values, so basically to cover (value === null || value === undefined) with just ( value == null ) as it will return true for undefined also. Let me know if you think we should explicitly check for undefined with strict equality, I will make the change.
There was a problem hiding this comment.
@jspannu919 yes, please. We try to use strict equality as much as possible to avoid some unintentional condition outcomes.
|
🎉 This PR is included in version 3.20.9 🎉 The release is available on |
Currently the numeric validators (min or max) doesn't shows any validation error if the value in the field is zero.
Scenario:
If a field is added with min validation with threshold as 3, the validator will show error for values like 1,2,-1 etc but nothing for 0.