Conversation
bee8c97 to
182c0da
Compare
Passing run #1415 ↗︎
Details:
Review all test suite changes for PR #749 ↗︎ |
|||||||||||||||
pakb
left a comment
There was a problem hiding this comment.
Small issue I see with this approach, the await means that the GeoJSON will be added as preview layer wherever is the mouse when this finishes.
There's a feature of axios called cancel token that should be leverage here, so that if the mouse goes out of the GeoJSON layer we cancel the pending request.
If you want to see this issue "live", search for "Temperature", quickly go trough "Temperature, 2m min" and end your move on any other layer (that is no GeoJSON) in the searchbar, after 1sec or more, the preview layer will be forced to the GeoJSON instead of the one currently under the mouse
Yea I just saw this issue, thanks for the axios tip I'll have a look |
182c0da to
3190e1c
Compare
Sometimes during the releases build the test "reads and adds an external WMTS correctly" reached the timeout of waitMapIsReady. It seems to occur only during release build strangely.
Jumping between category worked only when the previous category had inputs.
3190e1c to
3e7e737
Compare
3e7e737 to
89869a1
Compare
The preview of a search entry can be quite time processing and needed to have some debouncing to avoid too many store dispatch. Also if the preview was a geojson layer that needs to be loaded we needed a way to cancel the load if it was still ongoing when clearing the preview. To solve those issue the preview layer is delegated to the search list component which is responsible to send the preview dispatch. This also allow to avoid sending in a row a clear and set dispatch when changing focus from one entry to the other. Now we also have the preview when navigating with the keypboard. The geojson preview loading is also aborted when preview is cleared.
89869a1 to
3d09674
Compare
pakb
left a comment
There was a problem hiding this comment.
Small question, but overall looking good as it is
| async function loadAndUpdatePreviewLayer(store, layer) { | ||
| cancelLoadPreviewLayer() |
There was a problem hiding this comment.
I'm wondering if we shouldn't check which layer we are loading. Because if we are hover a GeoJSON and then clicking it very quickly, this means this will cancel the initial load of data/style for the layer, and we will fire it once more to show it definitively on the map...
I don't know if this would bring too much complexity here, your thoughts?
There was a problem hiding this comment.
@pakb good point, however this would add much complexity. Currently the preview layer object and the active layer object are two different object, therefore when the layer is added it reload the geojson data and style for the new object. We could in theory reuse the preview object instead of creating a new object, but I think this would complexify the code and could bring more trouble. If the geojson style and data have proper cache-control header (I check a few meteoswiss layers) then we don't have any performance issue as the second load is done by the browser cache.
For testing enter
ch.meteoschweiz.messwerte-niederschlagin the searchbar and hover the results.
Test link