-
Notifications
You must be signed in to change notification settings - Fork 27.4k
Ng model options fix #15401
Ng model options fix #15401
Conversation
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.
LGTM
(My two comments are non-blocking, but good-to-have 😃)
@@ -480,15 +480,37 @@ describe('ngModelOptions', function() { | |||
expect($rootScope.checkbox).toBeUndefined(); |
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.
Not directly related to this PR, but checkbox
should be name
(here and above).
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.
good spot
}); | ||
|
||
|
||
it('should trigger immediately for the default event if not listed in the debounce list', |
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.
This is not specific to the default
event. The bug affected any event that was not listed in debounce
as long as debounce
was an object and didn't contain the catch-all default
. I wouldn't mind adding one more assertion to the test, like this:
var inputElm = helper.compileInput(
'<input type="text" ng-model="name" name="alias" ' +
'ng-model-options="{' +
- 'updateOn: \'default blur\', ' +
+ 'updateOn: \'default blur foo\', ' +
'debounce: { blur: 5000 }' +
'}"' +
'/>');
helper.changeInputValueTo('a');
expect($rootScope.name).toEqual('a');
+ helper.changeInputValueTo('b');
+ browserTrigger(inputElm, 'foo');
+ expect($rootScope.name).toEqual('b');
});
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 trouble with the test as you suggest is that the default
event is triggered by helper.changeInputValueTo('b')
in any case. And so this would pass even if the foo
event had a weird truthy timeout.
7fde402
to
1adb3bd
Compare
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change?
Please check if the PR fulfills these requirements
Other information: