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

Custom search separator #424

Closed
wants to merge 2 commits into from
Closed

Conversation

xcolour
Copy link

@xcolour xcolour commented Jan 3, 2012

This patch makes the token used in search configurable and falls back on current token (" ") if not set.

In my project, I have a list filled with semi-hierarchical options that might look like:

app.cpu.load
app.mem.used
system.cpu.load
system.mem.used

And I'd like to be able to search for "cpu" and get the relevant entries.

This patch allows me to initialize a chosen field with

foo.chosen({search_separator: "."});

And get the desired behavior.

The second commit also enables the use of regexes as the separator. I find this very useful for defining multiple characters to tokenize on (e.g. make three tokens out of "foo:bar-baz" with /:|-/).

@@ -396,9 +396,9 @@ class Chosen extends AbstractChosen
if regex.test option.html
found = true
results += 1
else if option.html.indexOf(" ") >= 0 or option.html.indexOf("[") == 0
else if (@search_separator.test and @search_separator.test(option.html)) or option.html.indexOf(@search_separator) >= 0 or option.html.indexOf("[") == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're testing based on the regex, is there a reason to also check indexOf as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will running test repeatedly totally kill performance on very large selects (with at least several hundred elements)?

Copy link
Author

Choose a reason for hiding this comment

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

My intention here was to support both string separators and regex separators. So, the tests are:

Is this a regex (@search_separator.test) and does it match (@search_separator.test(option.html))?
or
Is this a string match (option.html.indexOf(@search_separator) >= 0)
or
I'm not actually sure, this test was already here. (option.html.indexOf("[") == 0)

Later on I use @search_separator again as an argument to .split(), but that takes strings and regexes.

@pfiller
Copy link
Contributor

pfiller commented Jan 13, 2012

I like the idea of this a lot, but wonder about performance of the newly introduced regex.

@xcolour
Copy link
Author

xcolour commented Jan 13, 2012

See my above comment on the indexOf() question.

I haven't tried the regex option on a long select, I'll test out the performance and get back to you.

@xcolour
Copy link
Author

xcolour commented Jan 13, 2012

Tested with 999 options, search_separator set to /,|\.|:|-|\//. No apparent performance hit for me, but that's on a new laptop with current versions of Chrome, Safari, and Firefox.

I stuck my test page in a gist if you want to test it for yourself: https://gist.github.com/49f6fbe128c878e804c4.

@sontek
Copy link

sontek commented Feb 9, 2012

This is a month old, any way to get it merged in?

@clutchski
Copy link

One more bump.

@aerichmond
Copy link

Having a custom search separator would also solve the #1063 (words with quotes or parenthesis) which was partially fixed by #212. While #212 (allow searching through the entire word) was a good feature it does not quite do the same thing as searching for words in quotes or parenthesis. It would be nice to be able to continue to search for full words but ignore specific characters. There is currently a line that ignores the square brackets "[]":

parts = option.html.replace(/\[|\]/g, "").split(" ");

It wold be great to have either a custom search separator regex or another option for specifying the regex of characters to ignore (maybe default to /[|]/).

@mrcasals
Copy link

Bumping this! :)

@pfiller
Copy link
Contributor

pfiller commented Sep 19, 2016

In an effort to try and get Chosen's repository under control and to best serve the people who use the tool, we're closing pull requests that add new functionality. For the time being, we're only merging bug fixes and functionality changes that are inline with Chosen's current scope. This doesn't mean we won't ever consider this functionality, but it does mean that we're being realistic about the time constraints the project maintainers have to give to this project.

Sorry this has sat open so long @xcolour. The hope is that by getting things under control, we can get to a place in the future where we can once again start adding new functionality (at which point we'd come back to closed PRs like this). Thanks for your patience and understanding!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants