Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

fix select: with ngOptions and ngMultiple #3230

Closed

Conversation

KJTsanaktsidis
Copy link

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!

@petebacondarwin
Copy link
Member

PR Checklist (Minor Bugfix)

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name)
  • PR doesn't introduce new api
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update (if suitable)
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@petebacondarwin
Copy link
Member

@KJTsanaktsidis - Have you signed the CLA and can you take a look at the commit message guidelines?

@KJTsanaktsidis
Copy link
Author

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.

@pkozlowski-opensource
Copy link
Member

@KJTsanaktsidis as for the commit message, it should convey what was fixed, so you should have something like:

fix(select): correctly handle ngMultiple and ngOptions combination or something along those lines. As for the body - detailed description is OK. What is missing is a reference to the issue it is fixing so you should have something like: Closes #2113 at the end of the commit's message body.

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
IgorMinar added a commit to IgorMinar/angular.js that referenced this pull request Jul 25, 2013
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
@IgorMinar IgorMinar closed this in d87fa00 Jul 25, 2013
@IgorMinar
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants