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

feature request: WMSTileLayer make url prop dynamic, replace vs extend params #247

Closed
5 tasks done
sunny-g opened this issue Nov 3, 2016 · 1 comment
Closed
5 tasks done

Comments

@sunny-g
Copy link

sunny-g commented Nov 3, 2016

Please make sure to check the following boxes before submitting an issue. Thanks!

  • Check that all peer dependencies are installed: React, ReactDOM and Leaflet.
  • Check that you are using a supported version of React (v0.14 or v15).
  • Check that you are using a supported version of Leaflet (v.0.7)
  • Make sure you have followed the quick start guide for Leaflet.
  • Make sure you have fully read the documentation and that you understand the technical considerations.

First, thanks for writing such a convenient library!

I want to recommend changing the url prop of a WMSTileLayer to be dynamic - currently, the layer won't update the url used to fetch the WMS layer, causing the new params to be sent to the wrong tile URL.

Unless I'm mistaken, the ability to change the URL dynamically would allow for declaratively rendering a map or set of maps without having to use the Leaflet Controls or having to resort to the imperative Leaflet API for adding and removing layers. Here's a potential implementation: #196 (comment)

In addition, in the interest of staying in alignment with React's declarative nature (though this contradicts the library's purpose of only "supporting features provided by Leaflet"), within componentWillUpdate, it would make sense to clear from the layer's params any params that do not exist in the newly received props before setting the params to the new props (of course, assuming they are distinct), along the lines of the solution provided here: Leaflet/Leaflet#3441 (comment)

Let me know what you think (more about the first recommendation than the second, though I think they both would be useful and are more intuitive than current behavior).

@PaulLeCam
Copy link
Owner

Yes, good idea. I added support for dynamic url prop in v1.0.0-rc.3.
Regarding the issue with unsetting other parameters, I think that should be handled by Leaflet directly so I'd rather not implement custom logic here. If you need to clear the layer you can always give it a different key.

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

No branches or pull requests

2 participants