Skip to content
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

fix: respect layers parameter in getFeatureInfoRequests #1865

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

jokd
Copy link
Contributor

@jokd jokd commented Oct 20, 2023

Fixes #1863
With this fix the multiselection tool will fully work and getfeatureinfo will exclude layers that are outside their zoom limits.

@jokd jokd changed the title respect layers parameter in getFeatureInfoRequests fix: respect layers parameter in getFeatureInfoRequests Oct 20, 2023
@jokd jokd marked this pull request as ready for review October 20, 2023 09:58
@Grammostola
Copy link
Contributor

The change appears what is suggested, very neat.
It seems to affect the pointer ms tool. I can't quite parse what you mean that the suggested change means in practice for what type of layers with the ms-tool (but I haven't detected any problem with the suggested change).

@jokd
Copy link
Contributor Author

jokd commented Oct 30, 2023

The change appears what is suggested, very neat. It seems to affect the pointer ms tool. I can't quite parse what you mean that the suggested change means in practice for what type of layers with the ms-tool (but I haven't detected any problem with the suggested change).

The layers parameter sent to the getFeatureInfoRequests function was never used before. Now if layers is set it will be used instead looping through queryableLayers in the function.

@Grammostola
Copy link
Contributor

Grammostola commented Oct 30, 2023

Yes. It might be very useful for other users of getFeaturesFromRemote, like the MS tool (based upon your suggestion). I can see that the intended changes apply to the pointer tool, I merely couldn't quite fathom what you meant with "out of zoom" and the practical application (for the MS tool) . But it has been a while since I tested the original MS plugin.

@jokd
Copy link
Contributor Author

jokd commented Oct 30, 2023

Yes. It might be very useful for other users of getFeaturesFromRemote, like the MS tool (based upon your suggestion). I can see that the intended changes apply to the pointer tool, I merely couldn't quite fathom what you meant with "out of zoom" and the practical application (for the MS tool) . But it has been a while since I tested the original MS plugin.

The shouldSkipLayer (https://github.com/origo-map/multiselect-plugin/blob/main/src/multiselect.js#L246) checks if layer is queryable, shouldn't be excluded according to config and is within zoom range

@Grammostola
Copy link
Contributor

Grammostola commented Oct 30, 2023

In the small map where I tried the MS with a WMS layer and the origo cities layer I did not see that function being called. I'm not sure that matters. If you are happy with this change then I'm happy. On the basis of it not seeming to have any adverse affects and employing a logical seeming change, it lgtm.

@jokd jokd merged commit 754cb1a into master Oct 31, 2023
4 checks passed
@jokd jokd deleted the rewrite-getFeaturesFromRemote branch October 31, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customize the getFeatureInfoRequests request
2 participants