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

Commit aad6095

Browse files
tboschjeffbcross
tbosch
authored and
jeffbcross
committed
refactor(select): reduce duplication and reorder functions
1 parent ab354cf commit aad6095

File tree

2 files changed

+130
-131
lines changed

2 files changed

+130
-131
lines changed

‎src/ng/directive/select.js‎

Lines changed: 108 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,24 @@ var ngOptionsMinErr = minErr('ngOptions');
7777
* used to identify the objects in the array. The `trackexpr` will most likely refer to the
7878
* `value` variable (e.g. `value.propertyName`). With this the selection is preserved
7979
* even when the options are recreated (e.g. reloaded from the server).
80+
81+
* <div class="alert alert-info">
82+
* **Note:** Using `selectAs` together with `trackexpr` is not possible (and will throw).
83+
* TODO: Add some nice reasoning here, add a minErr and a nice error page.
84+
* reasoning:
85+
* - Example: <select ng-options="item.subItem as item.label for item in values track by item.id" ng-model="selected">
86+
* values: [{id: 1, label: 'aLabel', subItem: {name: 'aSubItem'}}, {id: 2, label: 'bLabel', subItem: {name: 'bSubItemß'}}],
87+
* $scope.selected = {name: 'aSubItem'};
88+
* - trackBy is always applied to `value`, with purpose to preserve the selection,
89+
* (to `item` in this case)
90+
* - to calculate whether an item is selected we do the following:
91+
* 1. apply `trackBy` to the values in the array, e.g.
92+
* In the example: [1,2]
93+
* 2. apply `trackBy` to the already selected value in `ngModel`:
94+
* In the example: this is not possible, as `trackBy` refers to `item.id`, but the selected
95+
* value from `ngModel` is `{name: aSubItem}`.
96+
*
97+
* </div>
8098
*
8199
* @example
82100
<example module="selectExample">
@@ -345,7 +363,9 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
345363
// We try to reuse these if possible
346364
// - optionGroupsCache[0] is the options with no option group
347365
// - optionGroupsCache[?][0] is the parent: either the SELECT or OPTGROUP element
348-
optionGroupsCache = [[{element: selectElement, label:''}]];
366+
optionGroupsCache = [[{element: selectElement, label:''}]],
367+
//re-usable object to represent option's locals
368+
locals = {};
349369

350370
if (nullOption) {
351371
// compile the element since there might be bindings in it
@@ -363,146 +383,109 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
363383
// clear contents, we'll add what's needed based on the model
364384
selectElement.empty();
365385

366-
selectElement.on('change', function() {
386+
selectElement.on('change', selectionChanged);
387+
388+
ctrl.$render = render;
389+
390+
scope.$watchCollection(valuesFn, scheduleRendering);
391+
scope.$watchCollection(getLabels, scheduleRendering);
392+
393+
if (multiple) {
394+
scope.$watchCollection(function() { return ctrl.$modelValue; }, scheduleRendering);
395+
}
396+
397+
// ------------------------------------------------------------------ //
398+
399+
function callExpression(exprFn, key, value) {
400+
locals[valueName] = value;
401+
if (keyName) locals[keyName] = key;
402+
return exprFn(scope, locals);
403+
}
404+
405+
function selectionChanged() {
367406
scope.$apply(function() {
368407
var optionGroup,
369408
collection = valuesFn(scope) || [],
370-
locals = {},
371409
key, value, optionElement, index, groupIndex, length, groupLength, trackIndex;
372-
410+
var viewValue;
373411
if (multiple) {
374-
value = [];
375-
for (groupIndex = 0, groupLength = optionGroupsCache.length;
376-
groupIndex < groupLength;
377-
groupIndex++) {
378-
// list of options for that group. (first item has the parent)
379-
optionGroup = optionGroupsCache[groupIndex];
380-
381-
for(index = 1, length = optionGroup.length; index < length; index++) {
382-
if ((optionElement = optionGroup[index].element)[0].selected) {
383-
value.push(getViewValue(optionElement, locals, collection));
384-
}
385-
}
386-
}
412+
viewValue = [];
413+
forEach(selectElement.val(), function(selectedKey) {
414+
viewValue.push(getViewValue(selectedKey, collection[selectedKey]));
415+
});
387416
} else {
388-
value = getViewValue(selectElement, locals, collection);
417+
var selectedKey = selectElement.val();
418+
viewValue = getViewValue(selectedKey, collection[selectedKey]);
389419
}
390-
ctrl.$setViewValue(value);
420+
ctrl.$setViewValue(viewValue);
391421
render();
392422
});
393-
});
423+
}
394424

395-
ctrl.$render = render;
425+
function getViewValue(key, value) {
426+
if (key === '?') {
427+
return undefined;
428+
} else if (key === '') {
429+
return null;
430+
} else {
431+
var viewValueFn = selectAsFn ? selectAsFn : valueFn;
432+
return callExpression(viewValueFn, key, value);
433+
}
434+
}
396435

397-
scope.$watchCollection(valuesFn, scheduleRendering);
398-
scope.$watchCollection(function () {
399-
var locals = {},
400-
values = valuesFn(scope);
401-
if (values) {
402-
var toDisplay = new Array(values.length);
436+
function getLabels() {
437+
var values = valuesFn(scope);
438+
var toDisplay;
439+
if (values && isArray(values)) {
440+
toDisplay = new Array(values.length);
403441
for (var i = 0, ii = values.length; i < ii; i++) {
404-
locals[valueName] = values[i];
405-
toDisplay[i] = displayFn(scope, locals);
442+
toDisplay[i] = callExpression(displayFn, i, values[i]);
406443
}
407444
return toDisplay;
445+
} else if (values) {
446+
// TODO: Add a test for this case
447+
toDisplay = {};
448+
for (var prop in values) {
449+
if (values.hasOwnProperty(prop)) {
450+
toDisplay[prop] = callExpression(displayFn, prop, values[prop]);
451+
}
452+
}
408453
}
409-
}, scheduleRendering);
410-
411-
if (multiple) {
412-
scope.$watchCollection(function() { return ctrl.$modelValue; }, scheduleRendering);
454+
return toDisplay;
413455
}
414456

415-
416-
function getSelectedSet() {
417-
var selectedSet = false;
457+
function createIsSelectedFn(viewValue) {
458+
var selectedSet;
418459
if (multiple) {
419-
var viewValue = ctrl.$viewValue;
420-
if (trackFn && isArray(viewValue) && !selectAs) {
460+
if (!selectAs && trackFn && isArray(viewValue)) {
461+
421462
selectedSet = new HashMap([]);
422-
var locals = {};
423463
for (var trackIndex = 0; trackIndex < viewValue.length; trackIndex++) {
424-
locals[valueName] = viewValue[trackIndex];
425-
selectedSet.put(trackFn(scope, locals), viewValue[trackIndex]);
464+
// tracking by key
465+
selectedSet.put(callExpression(trackFn, null, viewValue[trackIndex]), true);
426466
}
427467
} else {
428468
selectedSet = new HashMap(viewValue);
429469
}
470+
} else if (!selectAsFn && trackFn) {
471+
viewValue = callExpression(trackFn, null, viewValue);
430472
}
431-
return selectedSet;
432-
}
433-
434-
function getViewValue (el, locals, collection) {
435-
var key = el.val();
436-
var value;
437-
var calculateViewValue;
438-
439-
if (selectAsFn || trackFn) {
440-
calculateViewValue = function () {
441-
var getterFn = selectAsFn || trackFn;
442-
var wrappedCollectionValue = {};
443-
wrappedCollectionValue[valueName] = collection[key];
444-
wrappedCollectionValue[keyName] = key;
445-
446-
for (var i in collection) {
447-
if (collection.hasOwnProperty(i)) {
448-
locals[valueName] = collection[i];
449-
if (keyName) locals[keyName] = i;
450-
if (getterFn(scope, locals) ==
451-
getterFn(scope, wrappedCollectionValue)) {
452-
/*
453-
* trackBy should not be used for final calculation, because it doesn't
454-
* necessarily return the expected value.
455-
*/
456-
return (selectAsFn||valueFn)(scope, locals);
457-
}
458-
}
459-
}
460-
};
461-
} else {
462-
calculateViewValue = function() {
463-
locals[valueName] = collection[key];
464-
if (keyName) locals[keyName] = key;
465-
return valueFn(scope, locals);
466-
};
467-
}
468-
469-
if (multiple) {
470-
if (keyName) locals[keyName] = key;
471-
calculateViewValue();
472-
return (selectAsFn || valueFn)(scope, locals);
473-
}
474-
475-
if (key == '?') {
476-
value = undefined;
477-
} else if (key === ''){
478-
value = null;
479-
} else {
480-
value = calculateViewValue();
481-
}
482-
483-
return value;
484-
}
485-
486-
function isSelected(viewValue, locals, selectedSet) {
487-
var compareValueFn;
488-
if (selectAsFn) {
489-
compareValueFn = selectAsFn;
490-
} else if (trackFn) {
491-
var withValueName = {};
492-
withValueName[valueName] = viewValue;
493-
compareValueFn = trackFn;
494-
495-
viewValue = trackFn(withValueName);
496-
} else {
497-
compareValueFn = valueFn;
498-
}
499-
500-
var optionValue = compareValueFn(scope, locals);
473+
return function isSelected(key, value) {
474+
var compareValueFn;
475+
if (selectAsFn) {
476+
compareValueFn = selectAsFn;
477+
} else if (trackFn) {
478+
compareValueFn = trackFn;
479+
} else {
480+
compareValueFn = valueFn;
481+
}
501482

502-
if (multiple) {
503-
return isDefined(selectedSet.remove(optionValue));
483+
if (multiple) {
484+
return isDefined(selectedSet.remove(callExpression(compareValueFn, key, value)));
485+
} else {
486+
return viewValue == callExpression(compareValueFn, key, value);
487+
}
504488
}
505-
return viewValue === optionValue;
506489
}
507490

508491
function scheduleRendering() {
@@ -512,11 +495,10 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
512495
}
513496
}
514497

515-
516498
function render() {
517499
renderScheduled = false;
518500

519-
// Temporary location for the option groups before we render them
501+
// Temporary location for the option groups before we render them
520502
var optionGroups = {'':[]},
521503
optionGroupNames = [''],
522504
optionGroupName,
@@ -527,11 +509,12 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
527509
values = valuesFn(scope) || [],
528510
keys = keyName ? sortedKeys(values) : values,
529511
key,
512+
value,
530513
groupLength, length,
531514
groupIndex, index,
532-
locals = {},
533515
selected,
534-
selectedSet = getSelectedSet(),
516+
isSelected = createIsSelectedFn(viewValue),
517+
anySelected = false,
535518
lastElement,
536519
element,
537520
label;
@@ -542,24 +525,19 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
542525
if (keyName) {
543526
key = keys[index];
544527
if ( key.charAt(0) === '$' ) continue;
545-
locals[keyName] = key;
546528
}
529+
value = values[key];
547530

548-
locals[valueName] = values[key];
549-
550-
optionGroupName = groupByFn(scope, locals) || '';
531+
optionGroupName = callExpression(groupByFn, key, value) || '';
551532
if (!(optionGroup = optionGroups[optionGroupName])) {
552533
optionGroup = optionGroups[optionGroupName] = [];
553534
optionGroupNames.push(optionGroupName);
554535
}
555536

556-
selected = isSelected(
557-
viewValue,
558-
locals,
559-
selectedSet);
560-
selectedSet = selectedSet || selected;
537+
selected = isSelected(key, value);
538+
anySelected = anySelected || selected;
561539

562-
label = displayFn(scope, locals); // what will be seen by the user
540+
label = callExpression(displayFn, key, value); // what will be seen by the user
563541

564542
// doing displayFn(scope, locals) || '' overwrites zero values
565543
label = isDefined(label) ? label : '';
@@ -573,8 +551,8 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
573551
if (!multiple) {
574552
if (nullOption || viewValue === null) {
575553
// insert null option if we have a placeholder, or the model is null
576-
optionGroups[''].unshift({id:'', label:'', selected:!selectedSet});
577-
} else if (!selectedSet) {
554+
optionGroups[''].unshift({id:'', label:'', selected:!anySelected});
555+
} else if (!anySelected) {
578556
// option could not be found, we have to insert the undefined item
579557
optionGroups[''].unshift({id:'?', label:'', selected:true});
580558
}

‎test/ng/directive/selectSpec.js‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,7 @@ describe('select', function() {
665665

666666
describe('trackBy', function() {
667667
beforeEach(function() {
668-
scope.arr = [{id: 10, label: 'ten'}, {id:'20', label: 'twenty'}];
668+
scope.arr = [{id: 10, label: 'ten'}, {id:20, label: 'twenty'}];
669669
scope.obj = {'10': {score: 10, label: 'ten'}, '20': {score: 20, label: 'twenty'}};
670670
});
671671

@@ -1333,6 +1333,27 @@ describe('select', function() {
13331333
});
13341334

13351335

1336+
it('should update options in the DOM from object source', function() {
1337+
compile(
1338+
'<select ng-model="selected" ng-options="val.id as val.name for (key, val) in values"></select>'
1339+
);
1340+
1341+
scope.$apply(function() {
1342+
scope.values = {a: {id: 10, name: 'A'}, b: {id: 20, name: 'B'}};
1343+
scope.selected = scope.values.a.id;
1344+
});
1345+
1346+
scope.$apply(function() {
1347+
scope.values.a.name = 'C';
1348+
});
1349+
1350+
var options = element.find('option');
1351+
expect(options.length).toEqual(2);
1352+
expect(sortedHtml(options[0])).toEqual('<option value="a">C</option>');
1353+
expect(sortedHtml(options[1])).toEqual('<option value="b">B</option>');
1354+
});
1355+
1356+
13361357
it('should bind to object key', function() {
13371358
createSelect({
13381359
'ng-model': 'selected',

0 commit comments

Comments
 (0)