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

Change search_text to use option.text rather than option.html #1638

Merged
merged 1 commit into from
Mar 16, 2014

Conversation

simonexmachina
Copy link
Contributor

Ember.js litters the DOM with <script> tags that are used to perform its DOM binding. So when using an Ember.Select component with Chosen, searches would never return results because the regex binds to the start using /^..., and the value of option.html always starts with a <script> tag - so it would never match the regex.

This PR fixes the problem by searching on option.text. Another solution would be to change the regex to not bind to the start, but these seemed to be a less significant change.

@tjschuck
Copy link
Member

tjschuck commented Nov 6, 2013

@kenearley Of the @harvesthq/chosen-developers, you probably know the most about Ember as far as I know.

/cc @jkintscher, just because I know you've wrestled with Chosen + Ember.

@kenearley
Copy link

There has been a lot of issues opened on this topic. I think @pfiller is actually more familiar with the problem.

I think this is the main thread: #1150

@simonexmachina
Copy link
Contributor Author

Okay, I'm surprised I didn't find that issue - strange. Okay good, I'll close this one then.

@pfiller
Copy link
Contributor

pfiller commented Nov 7, 2013

@aexmachina Thanks for opening this PR. It's a good kick in the pants to get this fixed. With the popularity of Ember rising, it seems a worthwhile endeavor to make Chosen work more easily (especially if it comes at such a low cost). If @stof's comment on #1150 is right, this would certainly be merge-ready.

@simonexmachina
Copy link
Contributor Author

That was certainly my impression as I worked through the source.

@tjschuck
Copy link
Member

Seems like this would be the solution to #1150, right? Since that's just an issue and this is an actual pull request, I'm going to re-open it as the Official Proposed Solution™, since some dupes are coming in now, like #1661.

I'm also going to add this magic language which hopefully GitHub will pick up even though it's not by the official poster:

Fixes #1150

@mattbeedle
Copy link

Is this going to be merged any time soon? I just ran into this issue with emberjs.

@alexspeller
Copy link

It seems like lots of people are having the same issue - I just filed an identical pull request to this (#1661). Any reason not to merge this?

@alexspeller
Copy link

@tjschuck what needs to happen to get this merged? If there is anything needed, I will happily do it. This 4-character change has dragged on long enough, no?

@tjschuck
Copy link
Member

@alexspeller Sorry, I mostly just triage and police issues. The @harvesthq/chosen-developers need to give the final say.

I think whether or not this is "safe" per the conversation on #1150 is the only outstanding thing that has kept this from being merged so far.

@koenpunt
Copy link
Collaborator

I'm not really familiar with the problem, or the solution for that matter. But I think @stof or else @pfiller can push the button?

stof added a commit that referenced this pull request Mar 16, 2014
Change `search_text` to use `option.text` rather than `option.html`
@stof stof merged commit 35bde75 into harvesthq:master Mar 16, 2014
@tjschuck
Copy link
Member

Release notes updated.

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.

9 participants