Skip to content
This repository has been archived by the owner on Oct 2, 2019. It is now read-only.

fix(uiSelectMultiple): Don't call onSelectCallback if limit already reached #1836

Merged
merged 3 commits into from
Nov 3, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 1 addition & 12 deletions src/uiSelectController.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ uis.controller('uiSelectCtrl',
ctrl.refreshing = false;
ctrl.spinnerEnabled = uiSelectConfig.spinnerEnabled;
ctrl.spinnerClass = uiSelectConfig.spinnerClass;

ctrl.removeSelected = uiSelectConfig.removeSelected; //If selected item(s) should be removed from dropdown list
ctrl.closeOnSelect = true; //Initialized inside uiSelect directive link function
ctrl.skipFocusser = false; //Set to true to avoid returning focus to ctrl when item is selected
Expand Down Expand Up @@ -433,17 +432,7 @@ uis.controller('uiSelectCtrl',
}
_resetSearchInput();
$scope.$broadcast('uis:select', item);

var locals = {};
locals[ctrl.parserResult.itemName] = item;

$timeout(function(){
ctrl.onSelectCallback($scope, {
$item: item,
$model: ctrl.parserResult.modelMapper($scope, locals)
});
});


if (ctrl.closeOnSelect) {
ctrl.close(skipFocusser);
}
Expand Down
9 changes: 9 additions & 0 deletions src/uiSelectMultipleDirective.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,15 @@ uis.directive('uiSelectMultiple', ['uiSelectMinErr','$timeout', function(uiSelec
return;
}
$select.selected.push(item);
var locals = {};
locals[$select.parserResult.itemName] = item;

$timeout(function(){
$select.onSelectCallback(scope, {
$item: item,
$model: $select.parserResult.modelMapper(scope, locals)
});
});
$selectMultiple.updateModel();
});

Expand Down
9 changes: 9 additions & 0 deletions src/uiSelectSingleDirective.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,15 @@ uis.directive('uiSelectSingle', ['$timeout','$compile', function($timeout, $comp

scope.$on('uis:select', function (event, item) {
$select.selected = item;
var locals = {};
locals[$select.parserResult.itemName] = item;

$timeout(function(){
$select.onSelectCallback(scope, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is scope the correct scope here? Is there a $select.scope it should be instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some checking with the unit test if this is the case but then it will fail. Why do you think it should be $select.scope?

Copy link
Contributor

@user378230 user378230 Nov 2, 2016

Choose a reason for hiding this comment

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

My only thought was that it wasn't the same scope as the one in uiSelectController but I guess we don't use a new scope anyway for the uiSelectSingle/Multiple directives so it should be ok...

$item: item,
$model: $select.parserResult.modelMapper(scope, locals)
});
});
});

scope.$on('uis:close', function (event, skipFocusser) {
Expand Down
47 changes: 47 additions & 0 deletions test/select.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1819,6 +1819,8 @@ describe('ui-select tests', function() {
if (attrs.lockChoice !== undefined) { matchesAttrsHtml += ' ui-lock-choice="' + attrs.lockChoice + '"'; }
if (attrs.removeSelected !== undefined) { attrsHtml += ' remove-selected="' + attrs.removeSelected + '"'; }
if (attrs.resetSearchInput !== undefined) { attrsHtml += ' reset-search-input="' + attrs.resetSearchInput + '"'; }
if (attrs.limit !== undefined) { attrsHtml += ' limit="' + attrs.limit + '"'; }
if (attrs.onSelect !== undefined) { attrsHtml += ' on-select="' + attrs.onSelect + '"'; }
}

return compileTemplate(
Expand Down Expand Up @@ -2764,6 +2766,51 @@ describe('ui-select tests', function() {
expect(el.scope().$select.selected.length).toBe(2);
});

it('should set only 1 item in the selected items when limit = 1', function () {
var el = createUiSelectMultiple({limit: 1});
clickItem(el, 'Wladimir');
clickItem(el, 'Natasha');
expect(el.scope().$select.selected.length).toEqual(1);
});

it('should only have 1 item selected and onSelect function should only be handled once.',function(){
scope.onSelectFn = function ($item, $model) {
scope.$item = $item;
scope.$model = $model;
};
var el = createUiSelectMultiple({limit:1,onSelect:'onSelectFn($item, $model)'});
Copy link
Contributor

Choose a reason for hiding this comment

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

minor tidy up of your spacing here.. 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the bracket?


expect(scope.$item).toBeFalsy();
expect(scope.$model).toBeFalsy();

clickItem(el, 'Samantha');
$timeout.flush();
clickItem(el, 'Natasha');
$timeout.flush();
expect(scope.selection.selectedMultiple[0].name).toBe('Samantha');
expect(scope.$model.name).toEqual('Samantha');
expect(el.scope().$select.selected.length).toEqual(1);
});

it('should only have 2 items selected and onSelect function should be handeld.',function(){
scope.onSelectFn = function ($item, $model) {
scope.$item = $item;
scope.$model = $model;
};
var el = createUiSelectMultiple({onSelect:'onSelectFn($item, $model)'});

expect(scope.$item).toBeFalsy();
expect(scope.$model).toBeFalsy();

clickItem(el, 'Samantha');
$timeout.flush();
expect(scope.$model.name).toEqual('Samantha');
clickItem(el, 'Natasha');
$timeout.flush();
expect(scope.$model.name).toEqual('Natasha');
expect(el.scope().$select.selected.length).toEqual(2);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to check the value for scope.$model.name?


describe('resetSearchInput option multiple', function () {
it('should be true by default', function () {
expect(createUiSelectMultiple().scope().$select.resetSearchInput).toBe(true);
Expand Down