Skip to content
New issue

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

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

Already on GitHub? # to your account

Add an option to disable search in each word. #824

Merged
merged 3 commits into from
Dec 5, 2012

Conversation

Goutet
Copy link
Contributor

@Goutet Goutet commented Sep 19, 2012

Allow to disable the search on each separate word in an option text.
All js files generated using cake and uglify.

@@ -410,6 +410,7 @@ class Chosen extends AbstractChosen

results = 0

split_words = !this.options.do_not_split
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should handle the option in AbstractChosen like others, to allow handling the default value.

And do_not_split does not seem a good name to me. What it is about ? Existing options related to configuring the search are all using search in their name (disable_search, search_contains, disable_search_threshold)

@Goutet
Copy link
Contributor Author

Goutet commented Sep 28, 2012

When searching search text in each option html, chosen first runs a regex on the whole option html, then , if not found on each separate word. The aim of the option is to disable this second search pass when not necessary or even unwanted.
So I suggest to use "@options.disable_search_split_words" as option name with default value false (usual behaviour).
If you agree with the name, I will push the changes.
Thanks

@@ -30,6 +30,7 @@ class AbstractChosen
@allow_single_deselect = if @options.allow_single_deselect? and @form_field.options[0]? and @form_field.options[0].text is "" then @options.allow_single_deselect else false
@disable_search_threshold = @options.disable_search_threshold || 0
@disable_search = @options.disable_search || false
@search_split_words = not (@options.disable_search_split_words || false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little awkward, but I think that's my fault. Given how other options work, I think this is a pretty fair compromise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

why not calling the option enable_search_split_words or something like that ? we already have afffirmative names, for instance allow_single_deselect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose "disable" in order to keep current behaviour on default false value. But as noted, it can be changed, considering also other search options like @search_contains.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree - we could have done this a little simpler. Opened a PR #940

@pfiller pfiller merged commit da35384 into harvesthq:master Dec 5, 2012
@pfiller
Copy link
Contributor

pfiller commented Dec 5, 2012

Thanks @Goutet. This is an interesting option that might help improve performance for some users. I wonder if it should always be true if @search_contains is on. Something to play with...

pfiller added a commit that referenced this pull request Dec 12, 2012
- #824, #940 Add an option to disable searching split words
- #917 Add an option to inherit initial select classes
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants