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

Issue 1943 allow jscode in options #2029

Merged

Conversation

hansthen
Copy link
Collaborator

@hansthen hansthen commented Nov 8, 2024

Boring but big. The leaflet class hierarchy allows all classes to have optional arguments. These optional arguments can contain JavaScript code or references to other Leaflet elements.

To support this in Folium I replaced (almost) all instances of parse_options in the constructors with this.options | tojavascript in the templates. I left the parse_options in case someone outside of Folium uses it.

@hansthen hansthen requested a review from Conengmo November 8, 2024 05:51
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.

Boring but big 😄 Kudos for taking this on. It looks good! I have two small comments, plus some suggestions to remove added print statements from tests. Other than that looks good!

hansthen and others added 5 commits November 17, 2024 15:20
Co-authored-by: Frank Anema <33519926+Conengmo@users.noreply.github.com>
Co-authored-by: Frank Anema <33519926+Conengmo@users.noreply.github.com>
Co-authored-by: Frank Anema <33519926+Conengmo@users.noreply.github.com>
Co-authored-by: Frank Anema <33519926+Conengmo@users.noreply.github.com>
Co-authored-by: Frank Anema <33519926+Conengmo@users.noreply.github.com>
@hansthen hansthen requested a review from Conengmo November 17, 2024 14:22
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.

Let's go! 🚀

@hansthen
Copy link
Collaborator Author

@Conengmo Perhaps you can also have a look at the raster_layers.WmsTileLayer class. I still use {{ this.options|tojson }} there because there is an option that should remain lowercase. The inconsistency bugs me (a bit). I could not think of a way to make it consistent. But perhaps you have suggestions on how we could use tojavascript here?

@Conengmo
Copy link
Member

Yes, I noticed that one as well. Can't think of a good, simple solution right now, so maybe what you did is best for now.

@hansthen hansthen merged commit f2ec195 into python-visualization:main Nov 17, 2024
11 checks passed
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.

2 participants