Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Ng model options fix #15401

Closed

Conversation

petebacondarwin
Copy link
Contributor

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:

Copy link
Member

@gkalpak gkalpak left a 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();
Copy link
Member

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

Copy link
Contributor Author

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',
Copy link
Member

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');
        });

Copy link
Contributor Author

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.

ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants