Fixed revalidateAndAttachOnBlur method to handle zeroes appropriately #178
Conversation
| @@ -179,7 +179,7 @@ | |||
| } | |||
|
|
|||
| // invalidate field before doing any validation | |||
| if((value === "" || value === null || typeof value === "undefined") || commonObj.isFieldRequired() || _validateOnEmpty) { | |||
| if((value !== "" || value !== null || typeof value !== "undefined") || commonObj.isFieldRequired() || _validateOnEmpty) { | |||
| ctrl.$setValidity('validation', false); | |||
There was a problem hiding this comment.
I'm not sure why you are inversing the check from === to !==, that's a very big 360 from actual code. Also, not sure why I can see 2 commits attached to this PR but at the same time it shows only 1 file changed... very strange.
There was a problem hiding this comment.
There are two commits with this because of line 182. We had discussed that !!value is probably not the correct check here, because it handles a value of 0 as a falsy value.
My first commit, I changed !!value and replaced it with the different method for checking if a value exists...
(value === "" || value === null || typeof value === "undefined")
...but then I realized it needed to be negated in this case. !!value essentially means if value exists, enter the body of this if block. So that we handle 0 correctly, we need to check that value exists by making sure it is not "", null or undefined.
The second commit fixes my mistake and negates the condition on line 182 so it keeps the same meaning as the original !!value.
Is that correct here?
There was a problem hiding this comment.
Okay, I have to apologize.
Yes, the !!value should be replaced by the more verbose check that value is not "", null, or undefined, but the condition should use &&s instead of ||s to properly negate the expression as a whole (otherwise it will always be true).
I understand there are multiple mistakes in this pull request. If you'd like to close this one, I can submit a "cleaner" one that should be correct from the start. My apologies, I think I got too excited to contribute to the repository and was being too hasty with my changes!
There was a problem hiding this comment.
haha don't be too rude on yourself, it's all good. 😉
I was just wondering why the inversion (=== to !==) as I thought it was an inversion of my code, and that would be a no no. But now I understand it was all your code from the start and yes it makes sense to do the !== with the &&
The first PR is always stressful, but don't worry, it's all cool. I will probably merge tonight or tomorrow. 🙊
There was a problem hiding this comment.
Perfect! Thanks for the help and support!
Fix to properly negate expression.
Fix for issue #177.