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 a custom container to the realtime plugin #1869

Merged
merged 7 commits into from
Mar 6, 2024

Conversation

hansthen
Copy link
Collaborator

@hansthen hansthen commented Feb 4, 2024

Add the container parameter from the leaflet realtime plugin to the Folium realtime plugin. So far, only tested with MarkerCluster. Also add tests and improved the documentation.

@hansthen
Copy link
Collaborator Author

hansthen commented Feb 5, 2024

pre-commit.ci autofix

@Conengmo Conengmo self-requested a review February 19, 2024 16:21
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.

Nice addition to this plugin! I've added 3 comments, hope you have time to address them. Afterwards we can get this merged!

folium/plugins/realtime.py Show resolved Hide resolved
folium/features.py Outdated Show resolved Hide resolved
folium/plugins/realtime.py Show resolved Hide resolved
@hansthen hansthen requested a review from Conengmo March 5, 2024 20:25
@Conengmo
Copy link
Member

Conengmo commented Mar 6, 2024

This looks good Hans, the docstring is pretty clear so I hope users can work with this.

More longterm, we can add LayerGroup as an abstract base class to indicate something that will generate a L.LayerGroup javascript object. That way we can make the Python class hierarchy mimic the Leaflet class hierarchy.

Though that does make sense, I'm hesitant from a complexity perspective. Adding a core Folium class doesn't seem to weigh up to facilitating a single type hint for an optional argument in a single plugin. Having too many layers of class inheritance makes things more complicated to understand. So I think I'm fine with how it's solved now.

I'll go ahead and merge it now. Thanks for your contribution to Folium!

@Conengmo Conengmo merged commit 964cf97 into python-visualization:main Mar 6, 2024
12 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