search

gwtbootstrap / gwt-bootstrap

A GWT Library that provides the widgets of Bootstrap, from Twitter.
http://gwtbootstrap.github.com
406 stars 190 forks source link

Fix typeahead matcher so it works with spaces #510

Closed rkfg closed 10 years ago

rkfg commented 10 years ago

The default matcher breaks (i.e. no suggestions are shown) if user enters text with spaces because of the HTML-markup inserted in the item text. I'm not sure the matcher should get the highlighted version of the item so I made this workaround. Probably, there could be a better fix.

buildhive commented 10 years ago

GWT-Bootstrap » gwt-bootstrap #539 SUCCESS This pull request looks good (what's this?)

buildhive commented 10 years ago

GWT-Bootstrap » gwt-bootstrap #540 SUCCESS This pull request looks good (what's this?)

caarlos0 commented 10 years ago

LGTM.

@reinert any thoughts?

reinert commented 10 years ago

The workaround just smells too much. The problem - untreated string - should be solved in its root, somewhere earlier, not in the moment it is passed to the query matching process.

reinert commented 10 years ago

@rkfg we would appreciate if you improve your solution following the suggestions above.

caarlos0 commented 10 years ago

@reinert makes sense!

rkfg commented 10 years ago

@reinert I know that this hack is quite dirty but I can't find the root of the problem. The Typeahed code is pretty simple and the highlighter actually returns its input without any modification. Probably items are changed somewhere inside jQuery and if that's the case, it's not that easy to fix for me...

reinert commented 10 years ago

As you speculated, the real root of the problem is in bootstrap's plugin. See https://github.com/rkfg/gwt-bootstrap/blob/patch-1/src/main/java/com/github/gwtbootstrap/client/ui/Typeahead.java#L261

We usually don't fix twbs problems. They should be fixed there, not here. But as twbs v2 isn't supported anymore, we can accept fixes like this.

I suggest you move you fix to https://github.com/rkfg/gwt-bootstrap/blob/patch-1/src/main/java/com/github/gwtbootstrap/client/ui/Typeahead.java#L232 which would be "our root of the problem".

reinert commented 10 years ago

Actually, I have another suggestion. Create a public setter for the field matcherCallback, this way anyone can implement theirs matching rule. You can let your the fix inside the default MatcherCallback. BTW, create public setter for the fields updaterCallback and highlighterCallback too. Thanks!

rkfg commented 10 years ago

I don't understand, sorry. The public setters already exist for all these fields, public void setUpdaterCallback(UpdaterCallback updaterCallback), public void setHighlighterCallback( HighlighterCallback highlighterCallback) and public void setMatcherCallback(MatcherCallback matcherCallback). What should be added instead?

I've done my own Typeahead variant which sets the matcher in the constructor and use it meanwhile. Moving the hack to the selectionMatcher looks like a good idea though. I'll fix it, test and create the pull request tomorrow.

reinert commented 10 years ago

Gosh! I was blind! .