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

Add option to limit selected options in multiselect #530

Merged
merged 3 commits into from
Apr 20, 2012
Merged

Add option to limit selected options in multiselect #530

merged 3 commits into from
Apr 20, 2012

Conversation

peteruhnak
Copy link
Contributor

As addressed in issues #154 #463 #494 this adds option to limit number of selected options for multiselect.
$(select).chosen({ limit : 5 });

@Schyzophrenic
Copy link

Thank you for this. I am looking for the exact same feature. I hope it will be merged soon!

@n40i
Copy link

n40i commented Mar 23, 2012

Yes, thank you - looking forward to seeing this in a build!

@dmarynych
Copy link

Great! Hope this feature will be in main build.

@billmn
Copy link

billmn commented Apr 10, 2012

up!!

@@ -31,6 +31,7 @@ class AbstractChosen
@disable_search_threshold = @options.disable_search_threshold || 0
@search_contains = @options.search_contains || false
@choices = 0
@limit_choices = @options.limit || false
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 using a big number is preferable to false as a default. Ideally, we'd use the actual length of the options for the limit as a default, but I think using something like 9999 is probably safe for all practical purposes. Using a default of 999 would eliminate part of the conditional.

Copy link
Collaborator

Choose a reason for hiding this comment

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

putting a number as default value would require changing the way the default is applied, as using || would forbid forcing false as value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using false as a default complicates the logic when we test for it later. Using a number would allow one check: if @limit_choices >= @choices

@pfiller
Copy link
Contributor

pfiller commented Apr 13, 2012

Thanks - just a few quibbles and we can get this merged in.

I think the name property should not be called limit, but rather something like limit_selected_options. We don't want any ambiguity with the name.

I also think that an event should be triggered if the limit is hit so that a message could possibly be displayed. Maybe something like liszt:maxselected?

I also made a comment where the default is set.

@peteruhnak
Copy link
Contributor Author

I'll get into it. Speaking of ambiguity limit_selected_options sounds more like yes/no state so maybe max_selected_options?

@stof
Copy link
Collaborator

stof commented Apr 13, 2012

+1 for max_selected_options

@pfiller
Copy link
Contributor

pfiller commented Apr 14, 2012

Yeah, that sounds great.

@pfiller pfiller merged commit 0069c0e into harvesthq:master Apr 20, 2012
@pfiller
Copy link
Contributor

pfiller commented Apr 20, 2012

Thanks a lot, @Polycode. It's really nice to close 3 issues with one PR.

I don't think this is perfect, but I think it'll help people who are careful with using it. It's pretty confusing to just have the results not show up with no explanation to the user, but the event gives a nice way to handle that situation.

@peteruhnak
Copy link
Contributor Author

Thank you.

However I disagree about confusion - it's like having <input type="text" maxlength="...">. I feel that firing event once the limit is reached is too late, imo it's much better to have some kind of counter so user can immediately tell what's his current situation. Currently I can't think about any better usage of the liszt:maxselected than display dialog/alert with warning, but that is obtrusive. Or am I wrong?

@pfiller
Copy link
Contributor

pfiller commented Apr 20, 2012

Though maxlength on an input is developer-friendly, it's not particularly user friendly. Stopping a user's intended behavior without explaining why isn't a wonderful experience. "Not perfect" does not mean it's bad, though -- I'm happy to have this merged in.

I agree that the event is probably coming too late for the user. Using Chosen's current behavior It is definitely possible to monitor the number of elements selected.

$("#some_field").val().length

This is a good something to build off of going forward. Thanks again @Polycode

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.

7 participants