Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

Play nicely with formatters #568

Closed
wants to merge 1 commit into from
Closed

Play nicely with formatters #568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions src/typeahead/test/integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
angular.module('ui.bootstrap.typeahead.test', [])
Copy link
Author

Choose a reason for hiding this comment

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

I needed an example of a directive that would add to the $formatters pipeline.

Copy link
Member

Choose a reason for hiding this comment

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

OK, but just put it into the same file, no need to create one.

Copy link
Author

Choose a reason for hiding this comment

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

The spec file?

On Thu, Jun 27, 2013 at 10:17 AM, Pawel Kozlowski
notifications@github.heygears.comwrote:

In src/typeahead/test/integration.js:

@@ -0,0 +1,23 @@
+angular.module('ui.bootstrap.typeahead.test', [])

OK, but just put it into the same file, no need to create one.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/568/files#r4920664
.

Grant Callaghan
(650) 646-5195
grantcallaghan.com

Copy link
Contributor

Choose a reason for hiding this comment

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

You can just register it for the test only using $provide.

describe('ui.bootstrap.typeahead', function() {
  beforeEach(module('ui.bootstrap.typeahead', function($provide) {
    $provide.directive('formatter', function() {
      ...
    });
  });
});

Copy link
Author

Choose a reason for hiding this comment

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

Awesome. Definitely gaps in my knowledge around jasmine and testing. I'm glad you can point me in the right direction.

.directive('formatter', function(){
return {
require:'ngModel',
link:function(scope, elm, attrs, ngModelCtrl){
var prefix = attrs.fPrfx === undefined? '' : attrs.fPrfx;

ngModelCtrl.$formatters.unshift(function(viewVal){
viewVal = viewVal == null ? viewVal: prefix + viewVal;
return viewVal;
});

ngModelCtrl.$parsers.unshift(function(viewVal){
if (angular.isString(viewVal)) {
if (viewVal.charAt(0) === '$'){
viewVal = viewVal.slide(1);
}
}
return viewVal;
});
}
};
});
28 changes: 23 additions & 5 deletions src/typeahead/test/typeahead.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
describe('typeahead tests', function () {

beforeEach(module('ui.bootstrap.typeahead'));
beforeEach(module('ui.bootstrap.typeahead.test'));
beforeEach(module('template/typeahead/typeahead.html'));

describe('syntax parser', function () {
Expand Down Expand Up @@ -250,7 +251,7 @@ describe('typeahead tests', function () {
$scope.states = [{code: 'AL', name: 'Alaska'}, {code: 'CL', name: 'California'}];
$scope.result = $scope.states[0];

var element = prepareInputEl("<div><input ng-model='result' typeahead='state as state.name for state in states'></div>");
var element = prepareInputEl("<div><input ng-model='result.name' typeahead='state as state.name for state in states'></div>");
Copy link
Author

Choose a reason for hiding this comment

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

From my understanding, on an angular input, the value is typically tied to a property with a "primitive" value, i.e. string, int, bool, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is typical, but shouldn't be mandatory. We need to preserve how it used to work (compare it to the <select> directive.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but in the select directive the option label is what is displayed much like the value of the input element. However, the underlying model pointer is changed by the selection action in both the select directive and this change. The value, or label, is then updated to represent the current value of the model.

TLDR; I believe this behaves comparatively to the select directive.

Copy link
Member

Choose a reason for hiding this comment

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

@gcallaghan OK, I need to take a deeper look into it. Can't do this right now but should have a spare hour over the weekend. Thnx for the PR and bearing with me here!

Copy link
Member

Choose a reason for hiding this comment

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

@gcallaghan I'm sorry but looking into it once again I can definitively say that it is a breaking change that shouldn't be introduced. We need to be able to bind the whole object (and not only it properties) to the model.

In short - you shouldn't need to change those tests in your PR.

var inputEl = findInput(element);

expect(inputEl.val()).toEqual('Alaska');
Expand Down Expand Up @@ -406,14 +407,31 @@ describe('typeahead tests', function () {

$scope.states = [{code: 'AL', name: 'Alaska'}, {code: 'CL', name: 'California'}];

var element = prepareInputEl("<div><input ng-model='result' typeahead='state.code as state.name for state in states | filter:$viewValue'></div>");
var element = prepareInputEl("<div><input ng-model='result.name' typeahead='state.code as state.name for state in states | filter:$viewValue'></div>");
Copy link
Author

Choose a reason for hiding this comment

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

same as above(254)

var inputEl = findInput(element);

changeInputValueTo(element, 'Alas');
triggerKeyDown(element, 13);

expect($scope.result).toEqual('AL');
expect(inputEl.val()).toEqual('Alaska');
expect($scope.result.name).toEqual($scope.states[0].code);
expect(inputEl.val()).toEqual('AL');
});
});

describe('ngModelController pipelines', function (){
it('should update the input\'s value to reflect the output of the $formatters pipeline', function (){
var prefix = '$';
$scope.states = [
{code: 'AL', name: 'Alaska'},
{code: 'CL', name: 'California'}
];
$scope.result = $scope.states[0];
element = prepareInputEl("<div><input ng-model='result.name' formatter f-prfx='"+prefix+"' typeahead='state.name for state in states | filter:$viewValue'></div>"),
inputEl = findInput(element);

expect(inputEl.val()).toEqual(prefix + $scope.result.name);


});
});

Expand All @@ -433,4 +451,4 @@ describe('typeahead tests', function () {
});

});
});
});
18 changes: 8 additions & 10 deletions src/typeahead/typeahead.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position'])

//expressions used by typeahead
var parserResult = typeaheadParser.parse(attrs.typeahead);

//underlying model
var modelGet = $parse(attrs.ngModel);

//should it restrict model values to the ones selected from the popup only?
var isEditable = originalScope.$eval(attrs.typeaheadEditable) !== false;
Expand Down Expand Up @@ -150,22 +153,17 @@ angular.module('ui.bootstrap.typeahead', ['ui.bootstrap.position'])
return isEditable ? inputValue : undefined;
});

modelCtrl.$render = function () {
var locals = {};
locals[parserResult.itemName] = selected || modelCtrl.$viewValue;
element.val(parserResult.viewMapper(scope, locals) || modelCtrl.$viewValue);
selected = undefined;
};

scope.select = function (activeIdx) {
//called from within the $digest() cycle
var locals = {};
var model, item;
var locals = {}, model, item;

locals[parserResult.itemName] = item = selected = scope.matches[activeIdx].model;

model = parserResult.modelMapper(scope, locals);
modelCtrl.$setViewValue(model);
modelCtrl.$render();
modelGet.assign(originalScope, model);
Copy link
Author

Choose a reason for hiding this comment

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

Setting the underlying model causes the mismatch between model and $modelValue that fires the $formatters pipeline. https://github.com/angular/angular.js/blob/v1.1.4/src/ng/directive/input.js#L1046-1066

Copy link
Member

Choose a reason for hiding this comment

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

Exactly, this is the whole difficulty! :-)

selected = undefined;

onSelectCallback(scope, {
$item: item,
$model: model,
Expand Down