geocodeearth / autocomplete-element

A custom element for use with the Geocode Earth Autocomplete API.
https://geocode.earth
MIT License
8 stars 1 forks source link

add onFeatures event #21

Closed missinglink closed 2 years ago

missinglink commented 2 years ago

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:

mxlje commented 2 years ago
  • 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 commented 2 years ago

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 commented 2 years ago

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)
     }