-
Notifications
You must be signed in to change notification settings - Fork 6.7k
fix(datepicker): validate using ng-required attribute #4002
Conversation
@wesleycho, moving the discussion to this PR: In response to your comments, I had some questions. Would the proper fix be to:
|
Correct on both counts |
does this look good? |
@@ -616,6 +615,11 @@ function ($compile, $parse, $document, $position, dateFilter, dateParser, datepi | |||
|
|||
function validator(modelValue, viewValue) { | |||
var value = modelValue || viewValue; | |||
|
|||
if (!attrs.ngRequired && !value) { |
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.
Is this even necessary? Does this run if ngRequired is falsy?
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.
Yes, it works even if ngRequired is falsy. If ngRequired is falsy (i.e. anything other than ng-required=true), then the datepicker is not required. So, it considers an empty value valid in that case, but will check for date validity for any non-empty value.
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 wasn't saying just working - I was wondering whether the validator even runs if !attrs.ngRequired
. If it doesn't run, it wouldn't be necessary, but if it does, then this line is fine.
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 validator does run if !attrs.ngRequired
This LGTM, I will do a full check on Sunday and hopefully will merge it in shortly after. |
hey, is there any update on this? |
Sorry, haven't had the chance to check this out yet, as it requires a little more setup/evaluation than the other stuff I have looked at so far. I do intend on reviewing this thoroughly this week though. |
Looks good here. |
This is a solution for the the issue (#3862), where date is always required for datepicker, even when ng-required is set to false.
(todo: still need to add tests)