-
Notifications
You must be signed in to change notification settings - Fork 55
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
feature: infowindow for search #1920
Conversation
Use infowindow for search result if set
From code inspection it looks like there are still two cases where it is hardcoded to use overlay via the internal showFeatureInfo function instead of calling the function on featureInfo. I haven't figured out if it not possible to use the featureInfo-version of showFeatureInfo, but if it is possible it should be changed everywhere so it always obeys the setting and the internal showFeatureInfo can be removed. |
In that case the featureInfo.showFeatureinfo has to be changed to allow features that are not in a layer. Maybe that can be adressed in another issue? |
Ok, that was what I expected, just didn't have time too dig into the details but I remembered that there was something special about how search (mis)used featureInfo.render(). If it is a larger rewrite we could probably settle for this PR at the moment, but it would be nice fix all cases to avoid confusion. |
I think it's quite a rewrite to fix the other cases so I suggest we fix that in another issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
But...
The case in line 147 does not make any sense. The newly created feature does not have any attributes, so infowindow will be empty. But that was also the case before this PR. Maybe the intention was that all attributes should be read from search response? In that case it would make sense to point out the layer to get the correct attribute formatting. Well, at least it will obey the "infoWindow"-setting without breaking anything.
Overall I don't think that the documentation is describing the five cases correctly. For instance, case 2 seems impossible to achieve. #1420 would be welcome.
Yeah it is a bit messy but this PR aims to use the configured infowindow where possible. Further improvements can be addressed and solved in another PR, such as issue #1420 |
Fixes #1228
Use infowindow for search result if set