Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: can set a maximum of selected items [maxSelectedItems] when mult… #239

Merged

Conversation

benjaminlefevre
Copy link
Contributor

@benjaminlefevre benjaminlefevre commented Feb 6, 2018

…iple selection is activated

#181

@benjaminlefevre benjaminlefevre changed the title feat: can set a maximum of selected items [maxSelecedItems] when mult… feat: can set a maximum of selected items [maxSelectedItems] when mult… Feb 6, 2018
@benjaminlefevre benjaminlefevre force-pushed the max_selected_items_when_multiple branch 2 times, most recently from 3466cf4 to 68411f1 Compare February 6, 2018 21:42
@coveralls
Copy link

coveralls commented Feb 6, 2018

Coverage Status

Coverage increased (+0.07%) to 90.052% when pulling 6159db1 on benjaminlefevre:max_selected_items_when_multiple into f15fafc on ng-select:master.

Copy link
Member

@anjmao anjmao left a comment

Choose a reason for hiding this comment

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

Nice work 👍 Added few comments regarding code style, but if you don't have time let me know, I will merge it and fix.

@@ -188,4 +188,8 @@ export class ItemsList {
private get _lastSelectedItem() {
return this._selected[this._selected.length - 1];
}

isMaximumNumberOfSelectedItemsReached(): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Move this public method before all private methods.

Copy link
Member

Choose a reason for hiding this comment

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

I also would suggest renaming this method to less verbose, eg.: maxItemsSelected or similar

@@ -269,7 +274,7 @@ export class NgSelectComponent implements OnInit, OnDestroy, OnChanges, AfterVie
}

open() {
if (this.isDisabled || this.isOpen) {
if (this.isDisabled || this.isOpen || this.isMaximumNumberOfSelectedItemsReaches()) {
Copy link
Member

Choose a reason for hiding this comment

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

You can remove that method and use one prod itemsList directly

if (this.isDisabled || this.isOpen || this.itemsList.isMaximumNumberOfSelectedItemsReaches()) {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case it should be this.itemsList.isMaximumNumberOfSelectedItemsReached() then

@benjaminlefevre
Copy link
Contributor Author

I will fix the issues this evening after work, thank you for the review

@andreasfranz
Copy link
Contributor

Hi @anjmao,
I also thought of such a feature recently, because I would need it for my project too. I think the suggested solution of @benjaminlefevre as a certain drawback: From a user's point of view it hard to recognize why the select box is suddenly disabled (without any further hint in the user interface).

As using a single select behaves differently: Once you select a new option is just replaces the previous one. Maybe this could also be an option, just replace the last item in the list with the newly selected. I am not sure if this is a better behavior than disabling the select box.
What do you think?

@anjmao
Copy link
Member

anjmao commented Feb 7, 2018

@andreasfranz replacing last selected item can look a bit strange and user always can remove item from the list so he have a choose. I think current implementation is good and it would be even less clear if dropdown is opened but you can't select items because max items are reached. Best approach would be to add some kind of hint (label) for user that he can select only x items.

@benjaminlefevre
Copy link
Contributor Author

benjaminlefevre commented Feb 7, 2018

I made the fixes.

And yes, I made a choice, which seemed to me the most natural: not to replace the last item, but to block the selection of items.
It would be nice to find a visual trick to indicate the max is reached, I thought of a tooltip but what about the internationalization.... I think it could be done afterwards, since for the moment this is a functionality which will not be used a lot.

@anjmao anjmao merged commit f500654 into ng-select:master Feb 8, 2018
jakemdunn pushed a commit to jakemdunn/ng-select that referenced this pull request Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants