jquense / react-widgets

Polished, feature rich, accessible form inputs built with React
http://jquense.github.io/react-widgets/
MIT License
2.34k stars 395 forks source link

Dropdown/Multiselect: Pass `searchTerm` as prop of `ItemComponent` #125

Closed andru closed 9 years ago

andru commented 9 years ago

I'm using a custom filter function to allow fuzzy searching (e.g. re dro list matches react widgets dropdown list).

This kind of search needs to highlight the matching characters in matching results for it to make sense to the user... eg. react widgets dropdown list

I can't currently see any non-hacky way to get the current searchTerm in the ItemComponent. It would be great if it was passed down to it as a prop.

jquense commented 9 years ago

Hi there,

the best way to handle some like this is to use a higher order component and a closure to grab a reference to the search term, you can see me employ this technique in the selectList widget (https://github.com/jquense/react-widgets/blob/master/src/SelectList.jsx#L95). The select list needs to adjust the style of the itemComponent based on whether the item is disabled or not.

andru commented 9 years ago

Thanks for the code. This is more or less how I'm handling it already, using a closure.

I'm a fan of complexity through composition, but in this case it feels like I'm adding complexity to pass data to a child which I argue it should receive from it's parent.

In this case, an ItemComponent is only ever rendered if it is found to match a searchTerm, so it seems to me that the ItemComponent should be given the data which propagated it's render.

Highlighting the matching part of a search term is a pretty common design feature of drop-down lists, so capturing that data with a closure seems like a lot of complexity to introduce to avoid passing down a string via props.

Is there a downside I'm not seeing? Performance of handing down the string to (n) ItemComponent instances?

jquense commented 9 years ago

the issue is that it isn't as simple as passing a string prop down. The list is a completely separate component used in all the dropdowns, not just the multiselect, so it has no idea about searchTerm, assuming that there is a searchTerm prop, that the List should pass the ItemComponent requires the List to know about its parent (even if implicitly) which is a bad design choice as it couples List tightly to certain parent components. Alternatively I could pass all props from the parent, to the list, to the itemComponent but that is super messy and destroys nice API boundaries.

I don't actually think using a closure in this way is overly complex. Javascript is really good at closures and this is use case is exactly where closures shine as a feature. For what it's worth, I do this in the selectList and have had no problems with performance or unmanageable complexity, and it is a good bit more complicated than closing over for a string value

andru commented 9 years ago

Got it. Thanks for the explanation! That makes a lot of sense.