fix select: with ngOptions and ngMultiple #3230
Conversation
PR Checklist (Minor Bugfix)
|
|
@KJTsanaktsidis - Have you signed the CLA and can you take a look at the commit message guidelines? |
|
Mm, I read the commit message guidelines, didn't realise I needed brackets in the first line. It should read "fix(select): with ngOptions and ngMultiple", yes? Is the body of my commit message OK? I'm not sure how to fix it, should I amend my commit message locally and git push --force, or close this PR, rewrite history on my repo with the correct message, and then open another PR? Re the CLA, I've signed it, and my real name is Konstantinos Tsanaktsidis. Re the travis CI build, it failed with "Error: Could not find or load main class org.angularjs.closurerunner.NgClosureRunner". Not sure how I managed to do that or how to fix it! The tests all pass on my local machine except for that iso8061 date string one I mentioned above. |
|
@KJTsanaktsidis as for the commit message, it should convey what was fixed, so you should have something like:
It is probably easier to amend a commit and do forced push instead of opening a new PR - at least we will keep history of this conversation. Don't worry about Travis for the moment. |
Previously, <select ngMultiple='...'> would only work with hard-coded <option> elements underneath, not with an ngOptions attribute. This is because the boolean ng-multiple attribute is only actually applied inside a scope.$watch, which happens after the link function for select is called. The fix is to scope.$watch on the multiple attribute in the select directive, and refresh the directive when it changes. Closes angular#2113
changing the type of select box from single to multiple or the other way around at runtime is currently not supported and the two-way binding does odd stuff when such situation happens. we might eventually support this, but for now we are just going to not allow binding to select[multiple] to prevent people from relying on something that doesn't work. BREAKING CHANGE: binding to select[multiple] directly or via ngMultiple (ng-multiple) directive is not supported. This feature never worked with two-way data-binding, so it's not expected that anybody actually depends on it. Closes angular#3230
|
hi there, thanks for the PR. unfortunately the fix is quite hackish and doesn't address the main issue which is that currently our two-way data binding for select doesn't support runtime mutation of the select.multiple property. supporting this is possible but is a major undertaking considering that the data binding for the select element is one of the most complex ones already. while reviewing this I realized that by providing ngMultiple directive we made it look like this feature is supported, which was wrong. for this reason I removed the ngMultiple directive and support for direct binding via the select[multiple] attribute. (commit: 45affbe) quite honestly, it's quite hard for me to imagine what a UI that dynamically changes select from single to multiple at runtime looks like especially in the context of two way data binding. if this turns out to be a common use case, then I'm sure that we'll look into this again and reimplement the select directive in a way that will support runtime transition of from single select to multi select. thanks again for the PR |
This is, I think, a fix for issue #2113 (#2113)
Previously, would only work with hard-coded elements underneath, not with ngOptions attribute. This is because the boolean attribute is only actually applied inside a scope.$watch, which happens after the link function for select is called. The fix is to scope.$watch on the multiple attribute in the select directive, and refresh the directive when it changes. The problem is demonstrated with these two plunks: Before patch: http://plnkr.co/edit/YCiJYpx5tq0pdc8Bp7Nb?p=preview After patch: http://plnkr.co/edit/hyi91xNSgg3cQ96jCBPy?p=preview I've signed the online contributor licence form, but I'm not sure how to link it with this pull request. EDIT: At least on my machine, one of the unit tests fails: "filters date should support various iso8061 date strings with timezone as input". It came that way when I downloaded it before I made any changes, I promise!