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

add onFeatures event #21

Merged
merged 1 commit into from
Feb 2, 2022
Merged

add onFeatures event #21

merged 1 commit into from
Feb 2, 2022

Conversation

missinglink
Copy link
Contributor

@missinglink missinglink commented Jan 31, 2022

This PR adds a new event called 'features' which emits the current geojson array backing the UI. (it strips the FeatureCollection envelope, but that's not a big deal)

I would like to be able to expose the underlying data such that I can update a map with all the results currently shown in the UI, without something like this there is no way to expose the raw response data.

couple potential issues/improvements:

  • ensure this also emits empty array [] appropriately (ie. when the UI is empty)
  • it fires in all the right places, including not firing on discard requests
  • consider the name of the event (ie. features), I also considered results
  • consider the event body structure, currently event.detail is an Array of features, would we possibly want more info later which would necessitate an object?

@missinglink missinglink force-pushed the event-new-data branch 3 times, most recently from 320f743 to c23f950 Compare January 31, 2022 20:35
@mxlje
Copy link
Contributor

mxlje commented Feb 1, 2022

  • ensure this also emits empty array [] appropriately (ie. when the UI is empty)
  • it fires in all the right places, including not firing on discard requests

With the added [results] dependency for the effect this will be called every time the results object changes. This happens when setResults is called, which happens:

  1. when results come back from an API requests (this already accounts for discarded results)
  2. when the search box is cleared by a user (clears results)

I think we should add a third call as part of onSelectItem that also clears the results (setResults(emptyResults)) when the user selects an item, which would then fire the new event with an empty array.

  • consider the name of the event (ie. features), I also considered results

I think list could work too, but sticking with features works.

consider the event body structure, currently event.detail is an Array of features, would we possibly want more info later which would necessitate an object?

I think an array of features makes sense. The change event fires before anyway and can be used to keep track of the current input value, for example.

@missinglink
Copy link
Contributor Author

missinglink commented Feb 1, 2022

I think we should add a third call as part of onSelectItem that also clears the results (setResults(emptyResults)) when the user selects an item, which would then fire the new event with an empty array.

I had a think about this, I'm considering that not having this 'results' event on onSelectItem might be preferable, at least for my usecase.

What I'm going to do is show a map view with all the Features from the 'features' event, then once a 'select' event fires I'll just use that single Feature in the map view instead.

If we had a 'results' event fire with an empty array and also a 'select' event fire, it would be more difficult for me to handle this, plus the order of the events would tightly couple the two UIs, for better or worse.

The callee already has the option of clearing their copy of the Features on a 'select' event, so it sounds like it's more powerful to not have this additional call, although the README text "Dispatched when the GeoJSON features backing the UI change" isn't quite correct 🤔

@missinglink
Copy link
Contributor Author

missinglink commented Feb 1, 2022

agh I don't know, I can make it work on the listener side either way, I'll add this via rebase:

diff --git a/src/autocomplete/autocomplete.js b/src/autocomplete/autocomplete.js
index 9a9f8cf..a834151 100644
--- a/src/autocomplete/autocomplete.js
+++ b/src/autocomplete/autocomplete.js
@@ -102,6 +102,8 @@ export default ({

   // called user-supplied callback when an item is selected
   const onSelectItem = ({ selectedItem }) => {
+    setResults(emptyResults)
+
     if (typeof userOnSelectItem === 'function') {
       userOnSelectItem(selectedItem)
     }

@mxlje mxlje merged commit 74e4952 into main Feb 2, 2022
@mxlje mxlje deleted the event-new-data branch February 2, 2022 13:54
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.

None yet

2 participants