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

Don't winnow results when search is empty #1226

Closed
wants to merge 5 commits into from

Conversation

kenearley
Copy link

@pfiller Here's a better (working) solution to the winnow_results performance. It no longer winnows when first opening the select.

Before change (5000 items)
screen shot 2013-05-21 at 9 28 47 am

After change
screen shot 2013-05-21 at 9 53 40 am

Note: The big numbers in the second screenshot represent actual searching. The rest are just opening the select.

@kenearley
Copy link
Author

One thing I just noticed is that if you click outside the Chosen select, it clears the search. However, if you hit ESC it doesn't clear the search. We can probably handle that in another PR.

@kenearley
Copy link
Author

@pfiller I've fixed the highlighting now. There is still two cases where it is unnecessarily winnowing.

  • After searching and selecting, reopen Chosen select
  • After searching and closing without selecting, reopen Chosen select

The problem seems to be that we want to reset the dropdown AND clear the highlighting whenever Chosen is closed. However, these are in two different places.

Overall, this change improves performance for users who predominantly use the mouse to open and choose an item.

@@ -22,6 +22,7 @@ class Chosen extends AbstractChosen
@form_field_jq = $ @form_field
@current_selectedIndex = @form_field.selectedIndex
@is_rtl = @form_field_jq.hasClass "chzn-rtl"
@previousSearchText = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be set in the parent constructor instead ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes.

@pfiller
Copy link
Contributor

pfiller commented May 21, 2013

Memoization for the win! This seems like a good start to me. We need about 500 more of these performance improvements to make Chosen hum.

I'd like to do a little more click testing tomorrow, but I think this is probably a +1

@pfiller
Copy link
Contributor

pfiller commented May 22, 2013

@kenearley I fixed a small issue where the highlight wasn't being applied when opening Chosen up if the search wasn't running. I've found another issue where option groups aren't showing and that one seems more complicated. Down the rabbit hole we go...

To recreate

  1. Open up an example page
  2. Find the multiple select with groups example
  3. Click it!
  4. Note that the NFL divisions aren't showing up

@pfiller
Copy link
Contributor

pfiller commented Jul 10, 2013

@kenearley With #1339 landing, I don't think this is necessary any longer. Chosen's winnow method is super fast now and I'd rather not have another state tracking variable if we don't have a strong need. If you disagree, feel free to merge master (it's not a clean merge) and re-open.

@pfiller pfiller closed this Jul 10, 2013
@koenpunt koenpunt deleted the winnow-unless-empty branch September 20, 2016 10:36
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.

3 participants