Skip to content

Add Leaflet.encoded plugin: Enable creating PolyLine and Polygon from encoded string #1928

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

Merged

Conversation

achieveordie
Copy link
Contributor

@achieveordie achieveordie commented Apr 10, 2024

Fixes #1886

  • Create a plugin class called PolyLineFromEncoded, which accepts an encoded string to make a polyline. Also, add all the relevant docs/tests required for plugin approval.
  • Create a plugin class called PolygonFromEncoded, which works the same way as above, but for polygons.

FYI @hansthen

Copy link
Collaborator

@hansthen hansthen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is very clean and looks complete.

I made one small change (added a type annotation). Since this is my first PR review, I will ask for a second check from @Conengmo if he has time. But to me it looks okay to merge.

@hansthen hansthen requested review from hansthen and Conengmo April 10, 2024 18:22
@achieveordie
Copy link
Contributor Author

In hindsight, I'm also considering adding the second functionality the original author provides - Polygon.fromEncoded(). They will be in the same plugin file (renamed from polyline_encoded to encoded) but have separate test/doc files.

@hansthen
Copy link
Collaborator

In hindsight, I'm also considering adding the second functionality the original author provides - Polygon.fromEncoded(). They will be in the same plugin file (renamed from polyline_encoded to encoded) but have separate test/doc files.

Do you want to include this in this PR? If so, go ahead, but you can also do it in a later PR.

@achieveordie achieveordie changed the title Add plugin to visualize the polyline from an encoded string Add Leaflet.encoded plugin: Enable creating PolyLine and Polygon from encoded string Apr 13, 2024
@achieveordie
Copy link
Contributor Author

done @hansthen :)

Copy link
Collaborator

@hansthen hansthen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@achieveordie
Copy link
Contributor Author

How long before this can be merged? I'd like to use the 'official' feature :)

Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, well done! I added two small comments, hope you can take a look. Shouldn't take long to get this merged!

super().__init__(
encoded,
)
path_options(line=True, radius=None, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick response! This is not working yet, I suppose you mean to pass these to the super init? Also in the other class.

@achieveordie
Copy link
Contributor Author

@Conengmo I appreciate your input on the error. It was an oversight on my end. This PR should be good to go.

@Conengmo Conengmo merged commit 5086929 into python-visualization:main May 7, 2024
hansthen added a commit to hansthen/folium that referenced this pull request Jun 2, 2024
…om encoded string (python-visualization#1928)

* add plugin to visualize the polyline from an encoded string

* correct import in user guide

* Update polyline_encoded.py

Added type information to for argument to `__init__`

* rework encoded plugin to include PolygonFromEncoded

* include doc and tests for PolygonFromEncoded

* update doc to include the link of the algo

* use path_options instead of parse_options

* set path_options to an attribute

---------

Co-authored-by: Hans Then <[email protected]>
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.

Create a PolyLine object directly from the polyline string via the Leaflet.encoded plugin
3 participants