PopupViewer sample: check all results for popups before building new one #511
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
mstefarov
force-pushed
the
matvei/popup-sample
branch
from
9524df8
to
c783dd5
Compare
mstefarov
force-pushed
the
matvei/popup-sample
branch
from
c783dd5
to
48fed59
Compare
dotMorten
approved these changes
Jul 11, 2023
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.
Sample change only
When testing PopupViewer sample with different maps, I noticed that our example often returns different results from the AGOL web viewer — and different from what I expected the behavior to be.
Our existing code checks each result for popups but then immediately goes into fallback code and builds a popup from a GeoElement. This means we never get around to check sublayer results, or check the second result.
In contrast, the AGOL map viewer first checks ALL results/sublayers for directly-returned popups, and only then starts looking at GeoElements. I implemented similar behavior by splitting GetPopup into two methods groups: GetPopup (recursively checks for popups only) and BuildPopupFromGeoElement (recursively checks for geoelements and popup definitions).
Example of a webmap where this can be seen: https://maps.arcgis.com/apps/mapviewer/index.html?webmap=df8bcc10430f48878b01c96e907a1fc3
In AGOL clicking on a warning zone shows its proper popup:
In current toolkit sample, it shows a basic generated popup for the "US States" polygon (which happens to be the first GeoElement in results):
With this PR, we now match AGOL behavior:
