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

feat(removeSelected): Implement removeSelected property for multiple selects #1622

Merged
merged 1 commit into from
May 31, 2016

Conversation

dondi
Copy link
Contributor

@dondi dondi commented May 25, 2016

Expose and implement remove-selected="false" for multiple selects. This will disable a choice
in the dropdown of a multiple-select element instead of removing it.

Sample behavior:
screen shot 2016-04-18 at 6 07 42 pm

In the existing version, id would not have appeared in the dropdown because it is currently selected. This pull request adds the option removeSelected so that, if set to false, selected items still appear in the dropdown but are disabled (like in the screenshot).

…selects

Implement remove-selected="false" for multiple selects. This will disable a choice
in the dropdown of a multiple-select element instead of removing it.
@@ -325,9 +330,9 @@ uis.controller('uiSelectCtrl',
var isDisabled = false;
var item;

if (itemIndex >= 0 && !angular.isUndefined(ctrl.disableChoiceExpression)) {
if (itemIndex >= 0 && (!angular.isUndefined(ctrl.disableChoiceExpression) || ctrl.multiple)) {
Copy link
Contributor

@user378230 user378230 May 26, 2016

Choose a reason for hiding this comment

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

@dondi could you add an extra set of brackets here just to make the condition precedence clear?

Eg.

if (itemIndex >= 0 && ((!angular.isUndefined(ctrl.disableChoiceExpression) || ctrl.multiple)))

Or..?

if ((itemIndex >= 0 && (!angular.isUndefined(ctrl.disableChoiceExpression)) || ctrl.multiple))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the parentheses already disambiguate the precedence. The current code is shown below, with the matching middle parentheses spaced out for emphasis:

if (itemIndex >= 0 &&   (  !angular.isUndefined(ctrl.disableChoiceExpression) || ctrl.multiple  )   ) {

Those parentheses ensure that the || in the middle takes precedence. The suggestions made above would result in double-parentheses on the same expression (the disjunction in the first example; the entire condition in the second example).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I totally misread it, my bad! 😳

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, all good—thanks for reviewing!

@user378230 user378230 merged commit f39b868 into angular-ui:master May 31, 2016
@user378230
Copy link
Contributor

@dondi did you want to update the wiki with some docs? (add a note saying available v0.17.2+)

Thanks again 👍

@dondi
Copy link
Contributor Author

dondi commented Jun 2, 2016

Great, thanks! I've added a section on this to the wiki. I made it an example section instead of adding it to the attribute table because it really only applies for multiple. (I noticed that multiple itself is not in that attribute table at the beginning, so I took my cue from there.) Let me know if the writeup needs any changes. Thank you again!

@thomasmiddeldorp
Copy link

@user378230 Is there any indication on when this feature will be released?

@marcbachmann
Copy link
Contributor

marcbachmann commented Jun 29, 2016

Is it possible that this pr introduced a new bug in the normal dropdown? Wrong options get removed in my dropdown. After a reload they appear again.

dropdown

edit: when I add remove-selected="false" to my ui-select directive, everything works again.

@dondi
Copy link
Contributor Author

dondi commented Jun 30, 2016

Yes, a bug was indeed caught and the fix has since been merged, presumably to emerge in the next release. Please see #1672 and #1681 to see if these behaviors match what you are seeing. You can try a build from master to see if the latest code addresses what you’re seeing.

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.

5 participants