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

Add PolyLine Offset plugin #1080

Closed
FabeG opened this issue Mar 5, 2019 · 6 comments
Closed

Add PolyLine Offset plugin #1080

FabeG opened this issue Mar 5, 2019 · 6 comments
Labels
plugin This issue/PR is about an existing or new plugin

Comments

@FabeG
Copy link
Contributor

FabeG commented Mar 5, 2019

It would be nice to implement the PolyLine Offset plugin (can be found here : https:/bbecquet/Leaflet.PolylineOffset), in order to draw parallel polylines.
I can propose a PR if you think it might be interesting.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 6, 2019

@FabeG while I'm not against adding this to the folium plugin submodule you can create the plugin as a separete package. That would allow you to evolve the plugin faster, without waiting for folium releases and review process.

@FabeG
Copy link
Contributor Author

FabeG commented Mar 6, 2019

Thanks for your answer @ocefpaf.
Actually, I had just developped this plugin for my own needs (not too difficult: inheritance from PolyLine class, just adding an offset argument), and I was wondering if it could be also useful for other folium users. No problem then if you think integrating this plugin in the folium plugin submodule is not that interesting.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 6, 2019

No problem then if you think integrating this plugin in the folium plugin submodule is not that interesting.

It is not that it is uninteresting. It is about maintenance burden and quick development. At the early stages of folium we incorporated all the plugins and that makes it hard for us now to update the underlying JS libraries and still get all the code to pass the tests. In hindsight we should've done the same model as leaflet itself, where each plugin is a different package. I do encourage you to make that plugin a separate package, if not, maybe @Conengmo may be a better person to ask if we should add that to folium itself or not.

@Conengmo
Copy link
Member

Hmm I agree on making sure we don't overburden ourselves with features. But then I don't think most of our time goes to light-weight plugins. If we only provide a simple wrapper around a Javascript plugin, we don't have that much to maintain, while providing possibly useful functionality. I also see the value of having a single authoritative package for discovery.

But some current plugins do make our lives hard. TimeSliderChoropleth comes to mind. Maybe we can define some criteria on which to decide whether new plugins are wanted. Here's a proposal:

  • the plugin provides interesting new functionality.
  • the wrapper template is simple.
  • the wrapper class has not much logic, just passing some things to the template.
  • no/little integration with other folium classes.

As well as these criteria for the process:

  • the contributor communicates well.
  • the first PR is of reasonably good quality.
  • the contributor can add a docstring, tests, entry in the plugins notebook and possibly a standalone example notebook.

@FabeG I think the plugin you link to is quite simple and won't make folium that much more complex. I'm thinking it could be a class that just calls (in JS) pl.setOffset() on a PolyLine. That seems simpler than inheriting and providing the same arguments twice.
So if you want to add this contribution to folium (including tests and example notebook) I'd be happy to review.

@FabeG
Copy link
Contributor Author

FabeG commented Mar 12, 2019

Hi @Conengmo, I submitted a PR (#1091), including tests and example notebook.
I created a PolyLineOffset class in the plugins folder (so not totally in line with your proposal, but at least it would allow us to start a discussion) that inherits from Polyline class, adding only an offset option.
A review would be appreciated :)

@Conengmo Conengmo added plugin This issue/PR is about an existing or new plugin work in progress Work is in progress on a PR, check the PR to see its status labels Mar 12, 2019
@Conengmo Conengmo removed the work in progress Work is in progress on a PR, check the PR to see its status label Oct 7, 2019
@Conengmo
Copy link
Member

This feature was merged in #1091

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin This issue/PR is about an existing or new plugin
Projects
None yet
Development

No branches or pull requests

3 participants