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

Pass selected listener dataset name. #207

Closed
wants to merge 1 commit into from

Conversation

jeffmax
Copy link
Contributor

@jeffmax jeffmax commented Apr 19, 2013

This pull request adds the dataset name to the suggestion object and passes it to "typahead:selected" listeners. The rational behind this change is that might be useful to know which dataset a given datum came from if it was a remotely loaded. An example use case would be reloading the choice from the correct datasource on page reload within a stateful app. The dataset parameter is just an additional parameter sent to listeners and should not break anything. See #203.

@jharding
Copy link
Contributor

Appreciate the pull request, but I'd prefer to tackle this issue using namspaced events. I'll reopen this pull request if I end up not going that route.

@jharding jharding closed this Apr 22, 2013
@jeffmax
Copy link
Contributor Author

jeffmax commented Apr 22, 2013

@jharding appreciate the consideration. I would just like to gently add one thing. Given the support for multiple datasets (which is a unique and nice feature) the source of a given datum becomes something of a first-class piece of information (in my use-cases at least). The namespaced events may be elegant in some situations, but in others, you will wind up having to bind an event for each datasource and possibly repeat code in each handler just to find out where the datum came from, on top of possibly binding to the main selected event if there are datums whose datasources you don't care about. In addition, I suspect (depending on implementation) the non-namespaced selected event will still fire for datums under watch for selection under a namespaced event, which could require awkward bookkeeping.
Either way, thanks for a great tool.

@bruth
Copy link

bruth commented Apr 22, 2013

👍 This would be very useful. I agree with @jeffmax.

@jharding In #196 you propose to add more data to the datum.. which again would require you to augment the data on the fly which is silly. I would argue that search APIs are intentionally very light on the data they return so they are fast. The two use cases are merging and handling multiple data sources as one.. in which case the data source would be most likely ignored and the other case is using the source to handle nuances of data from each source, but ultimately handling them the same way.

@jharding
Copy link
Contributor

Turns out namespaced events don't work like I thought they did so that's no longer an option. I'll reopen this and merge it into integration-0.9.3 later today.

@jharding jharding reopened this Apr 22, 2013
@jharding
Copy link
Contributor

Alright, merged into integration-0.9.3. Thanks again.

@ryanwinchester
Copy link

This should be in the docs? I have been trying all day to figure this out until I found this.

@burningTyger
Copy link

Looks like it's been pulled in the latest release. Any chance of adding it again?

@jharding
Copy link
Contributor

Hmm, yeah looks like a regression. Mind opening up another issue regarding this? I'll get it fixed for the next release (which should go out later this week).

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

Successfully merging this pull request may close these issues.

5 participants