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

fix(select): update labels #9061

Closed
wants to merge 1 commit into from
Closed

fix(select): update labels #9061

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
12 changes: 12 additions & 0 deletions src/ng/directive/select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,18 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
ctrl.$render = render;

scope.$watchCollection(valuesFn, scheduleRendering);
scope.$watchCollection(function () {
var locals = {},
values = valuesFn(scope);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: var values = valuesFn(scope);

if (values) {
var toDisplay = new Array(values.length);
for (var i = 0, ii = values.length; i < ii; i++) {
locals[valueName] = values[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure I'm just not seeing it, but where is valueName defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

nevermind, apparently was searching for nameValue --- so what are we doing, registering a $watchCollection in the middle of setup, for a single option?

Copy link
Contributor

Choose a reason for hiding this comment

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

What this is doing is saying, every digest loop, build a new array of all the display names, which are generated from expressions, which usually rely upon the valueName. Then if this array changes then re-render.

It is rather heavy weight - if we could say that these expressions only ever depended upon the values in the ng-option expression then it could be much simpler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the goal here to update labels dynamically though? in which case the values wouldn't matter much.

I think we could do a better job by doing the work in a single watcher, but it would be hard to read/reason about (the select directive is already pretty hard to read and reason about)

toDisplay[i] = displayFn(scope, locals);
}
return toDisplay;
}
}, scheduleRendering);

if (multiple) {
scope.$watchCollection(function() { return ctrl.$modelValue; }, scheduleRendering);
Expand Down
20 changes: 20 additions & 0 deletions test/ng/directive/selectSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,26 @@ describe('select', function() {
expect(element.val()).toEqual('1');
});

it('should update options in the DOM', function() {
compile(
'<select ng-model="selected" ng-options="item.id as item.name for item in values"></select>'
);

scope.$apply(function() {
scope.values = [{id: 10, name: 'A'}, {id: 20, name: 'B'}];
scope.selected = scope.values[0].id;
});

scope.$apply(function() {
scope.values[0].name = 'C';
});

var options = element.find('option');
expect(options.length).toEqual(2);
expect(sortedHtml(options[0])).toEqual('<option value="0">C</option>');
expect(sortedHtml(options[1])).toEqual('<option value="1">B</option>');
});


it('should bind to object key', function() {
createSelect({
Expand Down