-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Clear search input value, reset results when using hide_results_on_select option. #2867
Conversation
This looks good to me in general, but up to you if you want a better 👍 than mine. |
coffee/chosen.jquery.coffee
Outdated
unless @is_multiple && (!@hide_results_on_select || (evt.metaKey or evt.ctrlKey)) | ||
if @is_multiple && (!@hide_results_on_select || (evt.metaKey or evt.ctrlKey)) | ||
@search_field.val("") | ||
@winnow_results() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our more-or-less convention prescribes that functions should be accessed through this
, instead of @
.
So this.winnow_results()
in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I didn’t notice that convention! I’ll update and force-push.
coffee/chosen.proto.coffee
Outdated
unless @is_multiple && (!@hide_results_on_select || (evt.metaKey or evt.ctrlKey)) | ||
if @is_multiple && (!@hide_results_on_select || (evt.metaKey or evt.ctrlKey)) | ||
@search_field.value = "" | ||
@winnow_results() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
coffee/chosen.jquery.coffee
Outdated
@@ -372,7 +372,10 @@ class Chosen extends AbstractChosen | |||
else | |||
this.single_set_selected_text(this.choice_label(item)) | |||
|
|||
unless @is_multiple && (!@hide_results_on_select || (evt.metaKey or evt.ctrlKey)) | |||
if @is_multiple && (!@hide_results_on_select || (evt.metaKey or evt.ctrlKey)) | |||
@search_field.val("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we actually just always clear the value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing! We’re effectively always clearing the value — but I’ll move this statement out of the if-block to be more explicit about it.
Apart from my comment about "convention" I think this change is fine. |
On second thought, 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? |
The input value should be cleared when selecting a result, even if the dropdown isn’t hidden immediately (as per setting).
78c8933
to
39bd9c0
Compare
I can see how in some cases this might be useful, but this would be the only case where we’d not be clearing the value upon selecting things, which I think would lead to confusion. We’re actively attempting to use this with the value being cleared in Harvest (which is why I noticed this and opened a PR) — so I’m inclined to do what Harvest needs unless there’s strong feedback that it’s not what other people want. What do you think about that? Does it make sense to you? |
@koenpunt I’m hoping to push a feature in Harvest that relies on this changeset tomorrow — I feel pretty strongly that clearing the value is the right thing to do in this case. If you feel pretty strongly that it’s the wrong choice, let me know as soon as reasonable, otherwise I’ll keep this train-a-chugging! 🚂 |
Well like I said, this change would change existing behaviour, so I think it would only be reasonable to clear the input when there are no more results matching. Not sure if that is easily achievable? |
In all versions and options of Chosen, once a selection is made the input is cleared. I wasn't aware that this one option, introduced in #2687, persisted what you typed into the search box after making a selection. There doesn't seem to be discussion about the UX of this additional option, or any data points to suggest that this is how people prefer to use chosen when this option is present. Reading through #1546 I don't see people asking that what is typed in persists after a selection is made—the change customers are looking for is that the results dropdown stays open after a selection is made, so it's faster to make another selection. I would say it's more likely that the second or third or fourth selection a customer wants to make does not start with the same order of letters as the previous selection. Clearing the search input would be more beneficial to more users. Instead of having to delete what you just wrote in (which was probably only helpful to make the selection they just made), we'd do it for them, saving them a bunch of keystrokes. And for customers that need their next selection to start with the same order of letters as their previous selection (the vast minority), they can just type them in again. So I'm +1 to @adunkman's proposed interaction here. |
Maybe, maybe not. If people for example use Chosen for tags, and they have tags like Haven't investigated, but how hard would it be to check for the existence of matching unselected items? |
I don't disagree with you that this use-case can exist, but this is less likely to happen than someone wanting to select multiple options where the options don't have the same exact letters. And deleting what you typed in (for the majority use-case) is more effort than re-typing in the same thing (for the minority use-case). |
This is of course an assumption. I don't use this option in any way, so I won't be bothered by the potential adverse effect, so personally I don't really mind. |
It is, but it's not a wild assumption. There are many use-cases for selecting multiple options, and a use-case where a user wants to select multiple options that all happen to have the same order-of-letters in the options is just one of those use-cases. Keeping the results open after selecting an option is not directly related to this use-case. If you feel strongly that not clearing the input will be useful, it should be built as a separate configuration option for Chosen.
Behavior that was not discussed when implemented, and also built on an assumption.
👍 Thanks @koenpunt ! Again, if we hear from a lot of users that they'd prefer the input not be cleared, then it should be built as a separate option. |
True, but still behaviour that is present. I guess it is the reason we've been always very resilient/thoughtful in adding new features and/or options. Because adding stuff is way easier than removing it. |
Thanks for all the thoughtful discussion! It’s tough to make feature changes — but I do believe this was an accident when it was implemented. It felt like a bug when I came across it in usage. I’m hesitant to introduce a feature which doesn’t clear the input (including one which would conditionally clear depending on the number of filtered options). If I had been aware that this was intended (or even accidentally included) when this option was introduced, I would have certainly pushed back against it — not because it wouldn’t be useful occasionally, but because it is an unexpected interaction. I’ll handle the related issues if this becomes A Thing That People Don’t Like, and we can always change our minds. For now I’ll merge this and continue onward — thanks for spending your precious brain cycles on this! |
I'm aware this is very old, so maybe no-one will see this comment, but I wonder if the 'best' solution would be that when a user selects something, the typed text ("alan" in the above example) is cleared from the input box, but the drop-down list remains visible with that same filter applied until the user types something else or loses the focus? That way the people who want to select multiple alans still have the ability to do so (as the filtered list is still present), whilst those who want to start looking for the next item to select don't have anything to delete (as the typed text has been cleared). |
@harvesthq/chosen-developers
This PR affects mutiple-select Chosen when using the
hide_results_on_select: false
option introduced in #2687, and fixes #2809.When
hide_results_on_select: true
(the default),show_search_field_default()
is called which clears the search input and the results are hidden (which then are re-rendered when displaying again). Great!However, when
hide_results_on_select: false
is set, those are not called — leaving the text in the search input as well as not re-rendering the results list (the cause of #2809). Here‘s a reproduction in the Chosen docs (changed to passhide_results_on_select: false
) which makes it a lot easier to follow:In the demo above, the input should be cleared (“alan” should no longer be typed in Chosen after selecting the result).
If we additionally pass
display_selected_options: false
, we can exactly reproduce #2809, demonstrating that we’re not correctly re-rendering the list of available options:In the demo immediately above, the input should be cleared (“alan” should no longer be typed in Chosen) and the results list should immediately not contain the item anymore (rather than having to re-filter to see the item disappear).
The fix is relatively easy — simply clear the search input value, and re-winnow results when the results list is not immediately hidden — which is what is done in this PR.
Let me know if you have any thoughts or questions, I’d be happy to explain further! ❤️