fix: plugin was loading geojson layers multiple times #3143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🚀 Deployed on https://pr-3143--dhis2-maps.netlify.app |
3 flaky tests on run #3070 ↗︎
Details:
|
|||||||||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Routes > navigation by url changes > navigates to and away from download page |
Screenshots
|
|
interpretations.cy.js • 1 flaky test • e2e-chrome-parallel-2.41
| Test | Artifacts | |
|---|---|---|
| Interpretations > view interpretation after creating a map |
Screenshots
|
|
layers/geojsonlayer.cy.js • 1 flaky test • e2e-chrome-parallel-2.40
| Test | Artifacts | |
|---|---|---|
| GeoJSON URL Layer > adds a geojson url layer |
Screenshots
|
|
Review all test suite changes for PR #3143 ↗︎
jenniferarnesen
force-pushed
the
fix/multiple-layer-reloads-in-plugin
branch
from
bf6e436
to
786beac
Compare
turban
approved these changes
Mar 11, 2024
jenniferarnesen
added a commit
that referenced
this pull request
Mar 18, 2024
Implements: https://dhis2.atlassian.net/browse/DHIS2-15981 If geojson external overlay layers are configured for instance, they can now be added to a map. The data table will display the layer data as long as the collection is of homogenous geo types. User can also click on a feature and the right panel will open, showing the feature data. fix: check full instanceUrl and handle not response.ok in geojson loader (#3142) In production, the baseUrl is ".." and therefore the comparison will always be false. So check the instanceBaseUrl instead Also, a failed response from fetch, like 400, will return !response.ok, so that situation needed to be handled as well. fix: rename error to loadError to avoid name clash with building footprint ee layer (#3144) The building footprints earth engine layer has a property named error. That was getting picked up by the new notice box in the Card that was added to report geojson loading errors. Solution was to rename the error property in geojsonloader to loadError. fix: show no data message when geojson feature has no data (#3145) Geojson layers may or may not have data associated with them. Show an appropriate message if there is no data. fix: improve geojson layer error responses and cypress tests (#3149) fix: reduce the padding to make room for the scrollbar (#3148) fix: prevent data table effects from running code when no table (#3147) fix: map plugin - do not load async layers multiple times (#3143) fix: various fixes after release testing (#3151) * fix: limit stroke width to 0-10 * fix: tab length should only take the space of the title * fix: reset error when switching which layer shows data table * fix: highlight features when data table has filter * chore: improve legend for geojson layers * fix: set point radius to size that was set in the style settings * fix: the feature.id is set in maps-gl so use the properties.id instead find correct data * chore: legend item styling - set max line weight and rename to Point radius * fix: use more understandable error messages * fix: set minimum point size of 1 fix: final fixes for geojson (#3154) * fix: upgrade maps-gl for the rounded line join and caps * fix: add tooltip on layer thumbnails * chore: update i18n * fix: set map bounds after all layers are added to the map * fix: values that are strings but numeric in quality were not filtering * fix: onLayerAdded wasnt defined for SplitViews * Revert "fix: onLayerAdded wasnt defined for SplitViews" This reverts commit deeb327. * Revert "fix: set map bounds after all layers are added to the map" This reverts commit e1c0a58. * fix: make logic match prev code * fix: position tooltip right over the thumbnail title
dhis2-bot
added a commit
that referenced
this pull request
Mar 18, 2024
# [100.5.0](v100.4.1...v100.5.0) (2024-03-18) ### Bug Fixes * navigating by the changing url ignored download mode and interpretation id parameters ([#3125](#3125)) ([fcc5eaa](fcc5eaa)) * **translations:** sync translations from transifex (dev) ([#3132](#3132)) ([7f5d178](7f5d178)) * improve hover states and add consistent spacing [UX-161] ([#3121](#3121)) ([3f9e667](3f9e667)) ### Features * add ability to add GeoJSON URL external layers ([#3127](#3127)) ([fbdf0b0](fbdf0b0)), closes [#3142](#3142) [#3144](#3144) [#3145](#3145) [#3149](#3149) [#3148](#3148) [#3147](#3147) [#3143](#3143) [#3151](#3151) [#3154](#3154) * when rendering for push analytics, disable download button until map is rendered ([#3072](#3072)) ([4b1076c](4b1076c)), closes [#3121](#3121)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the LayerLoader was getting rerendered many times, because until all the layers were marked as loaded, the check on line 91 (old code) was true. This might not have been much of a problem before, but with the geojson layers having to fetch the data, there is a delay before all the layer data is received, so then the component gets rerendered in that time, firing another request for the geojson.
The bug I was seeing is that if there are multiple geojson layers on the map, the layers would flash each time the request was made. Potentially 20-30 times. In the worst case, it resulted in a javascript execution exception
geojson-endless-loading.mov
The change is to instead keep track of the loaded layers in a ref instead of state, which prevents the component from rerendering the LayerLoader when some layers are still loading. The component only needs to render twice: once when the layers are not loaded, and again once they are all loaded. This is kept track with a new state variable
mapIsLoadedafter:
geojson-loading-fix.mov