elfalem
commented
4 years ago Hello Garrett and thanks for the very detailed description. I agree with both changes and would love to incorporate them into the library.
For change 1, an empty function should be fine. If any issues arise in the future, recomputing the bounds should be easy to implement.
For change 2, it makes sense to adhere to the existing naming convention. I think it's possible to avoid a call to this.redraw() though since that's already done in this.setPath().
Something like:
setLatLngs: function(latlngs) {
return this.setPath(latlngs);
},
gsteelex
Hello, I'd like to bring to your attention 2 modifications that we have made to a fork of your library and ask if they are something you would like us to pull request back into your library.
The changes we made are to (1)add a function that is called internally from leaflet.Path.setStyle(), and (2)to add a function to Leaflet.curve that a library we are using references but does not exist in Leaflet.curve. We did not modify any existing functionality.
Background Information
We are using Leaflet.curve to draw a curve between 2 points on a map in a React application using react-leaflet and react-leaflet-ant-path to draw an animation along the curve. In our initial implementation without dynamic reloading of data on the map we are able to successfully integrate Leaflet.curve and the other 2 libraries. When we added dynamic reloading of the map source data we found that our map would error due to Leaflet.curve.setStyle() being called (which inherits from Leaflet.Path), and also error due to trying to call Leaflet.curve.setLatLngs() which was not implemented in Leaflet.curve.
We made the following changes to a fork of Leaflet.curve as we determined that would be the simplest changes we could make to use the libraries together.
Change 1: Add function called by Leaflet.Path.setStyle()
In the setStyle() method of Leaflet.Path, Leaflet.Path._updateBounds() is called and causes an error. The _updateBounds() method is not implemented in Leaflet.Path, Leaflet.Layer (parent to Leaflet.Path), or Leaflet.curve. We have added this function as an empty function to satisfy the method call, which appears to work for our purposes but may need further attention in the future.
Function calling _updateBounds:
Our change:
Change 2: Add function that is called by react-leaflet-ant-path
When passing Leaflet.curve to react-leaflet-ant-path as the line type to draw with an animation, react-leaflet-ant-path will eventually call Leaflet.curve.setLatLngs() as part of updating the rendered map once React has determined what to re-render. The function setLatLngs() is not in this library. The method setLatLngs() is not implemented in Leaflet.Path but is implemented by Leaflet.Polyline (which similarly takes a series of points along the line). While I think it could be said that for full support of Leaflet.curve to be claimed by react-leaflet-ant-path that library should handle Leaflet.curve differently and not expect the setLatLngs() method, I am instead posing the question "should Leaflet.curve.setLatLngs() exist since Leaflet.Polyline implements the exact method name and a number of other leaflet components implement a similarly named method?". With classes extending Polyline and other leaflet components using a very similar name I can also see it being beneficial for Leaflet.curve to follow this pattern and have a setLatLngs() method to potentially be more easily integrated with other libraries.
Separate functions that follow the naming convention of setLatLngs():
Our change:
We are interested to hear your thoughts on our changes and if you would like to include them in the Leaflet.curve library. If you would like to include the changes, I can create a fork branch to pull request with only the 2 specific function additions so as not to pick up the other changes I have made to allow us to reference the project as an npm dependency from GitHub.
Thank you in advance for your time.