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

Do not highlight first result or clear search input when holding control/meta key. #2910

Merged
merged 2 commits into from
Apr 9, 2018

Conversation

adunkman
Copy link
Contributor

@adunkman adunkman commented Oct 25, 2017

@harvesthq/chosen-developers to fix #2888, replaces #2900.

Building on @braddunbar’s #2900, this PR skips highlighting the first result as described in that PR and also does not reset the search input. This ensures we can do searchless multiple select when holding the control/meta key:

screencast of holding meta to select many options

This also ensures we can do multiple select while searching using the control/meta key:

screencast of searching and holding meta to select many options

This is similar to the suggestion that @koenpunt mentioned in #2867 (comment):

… it might be relevant to leave the text in, if someone wants to select multiple options that matched their query. So maybe we should only clear the input when there are no more matching options visible?

This PR doesn’t exactly implement this behavior — it instead leaves the input when meta/control is pressed — but allows users to achieve the same thing.

I wasn’t able to implement a prototype version of the related test — simulant doesn’t appear to bubble events, so I can’t get the event handler to trigger correctly in prototype (either the event comes from the correct .active-result item and doesn’t hit the event handler which is registered on .chosen-results, or the event comes from the wrong item (.chosen-results) and hits the event handler). I spent… too much time (3:04) working on trying to get a test working for prototype — but at least we’re covered with a jQuery test.

@tjschuck
Copy link
Member

I spent… too much time (3:04)

Thanks, Harvest! 😜

Copy link
Contributor

@satchmorun satchmorun left a comment

Choose a reason for hiding this comment

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

Tested it out before and after: looks good!

I'm not completely sold on the "leave the search string in on multiple select", but let's not let that hold us up!

🎉

@adunkman adunkman merged commit 2d6d468 into master Apr 9, 2018
@adunkman adunkman deleted the meta-key branch April 9, 2018 16:07
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.

List jumps to top after select with CMD
4 participants