Skip to content
This repository was archived by the owner on Jul 1, 2020. It is now read-only.

Fixed revalidateAndAttachOnBlur method to handle zeroes appropriately #178

Merged
merged 3 commits into from
Nov 14, 2018
Merged

Fixed revalidateAndAttachOnBlur method to handle zeroes appropriately #178

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b20b8f2
Fix for incorrect validation on "required" with a value of 0
mgm09a Nov 9, 2018
b249f4a
Update src/validation-directive.js
mgm09a Nov 9, 2018
134accc
Update src/validation-directive.js
mgm09a Nov 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/validation-directive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@
}

// invalidate field before doing any validation
if(!!value || commonObj.isFieldRequired() || _validateOnEmpty) {
if((value !== "" && value !== null && typeof value !== "undefined") || commonObj.isFieldRequired() || _validateOnEmpty) {
ctrl.$setValidity('validation', false);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@mgm09a mgm09a Nov 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect! Thanks for the help and support!

}

Expand Down Expand Up @@ -403,7 +403,9 @@
/** Re-evaluate the element and revalidate it, also re-attach the onBlur event on the element */
function revalidateAndAttachOnBlur() {
// Revalidate the input when enabled (without displaying the error)
var value = ctrl.$modelValue || '';
var value = ctrl.$modelValue !== null && typeof ctrl.$modelValue !== 'undefined'
? ctrl.$modelValue
: '';
if(!Array.isArray(value)) {
ctrl.$setValidity('validation', commonObj.validate(value, false));
}
Expand Down