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

Update VectorGridProtobuf to include popups when clicked #1933

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

christianaaronschroeder
Copy link

@christianaaronschroeder christianaaronschroeder commented Apr 18, 2024

I've tried using VectorGridProtobuf several times in the past and almost always found myself making a copy of this class and adding functionality to click a polygon and get the properties.

Open to suggestions, or other options, just putting this out there.

@christianaaronschroeder christianaaronschroeder marked this pull request as ready for review April 19, 2024 14:05
@hansthen
Copy link
Collaborator

@christianaaronschroeder Thanks for your contribution. This would be useful to add. I do have some questions and suggestions mostly having to do with configurability from the Python side.

Before going any further, there is an open Pull Request that does something similar. Could you have a look #1903?

It allows you to set event handlers (on) from the Python side. Would this also solve your needs?

@christianaaronschroeder
Copy link
Author

christianaaronschroeder commented Apr 22, 2024

@christianaaronschroeder Thanks for your contribution. This would be useful to add. I do have some questions and suggestions mostly having to do with configurability from the Python side.

Before going any further, there is an open Pull Request that does something similar. Could you have a look #1903?

It allows you to set event handlers (on) from the Python side. Would this also solve your needs?

I believe so! I'm open to trying that before moving forward with this one.

@hansthen
Copy link
Collaborator

@christianaaronschroeder Thanks for your contribution. This would be useful to add. I do have some questions and suggestions mostly having to do with configurability from the Python side.
Before going any further, there is an open Pull Request that does something similar. Could you have a look #1903?
It allows you to set event handlers (on) from the Python side. Would this also solve your needs?

I believe so! I'm open to trying that before moving forward with this one.

Great!

I will try to get that PR approved.

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.

Thanks for your PR Christian, great that you want to help improve Folium!

I understand this change is very useful for your situation, but I worry a bit about it applying to general users, since the use of a 'property' property as popup content is quite specific. I don't think that's a good fit, ideally interfaces are explicit (like the class parameters we often use) or are well documented. We also try to avoid custom code in plugins to avoid too much maintenance burden.

If we want to apply popups on this plugin, ideally it works with the Popup class Folium already has. That way we don't need custom code in this plugin.

Or take a look at the GeoJsonPopup class, which seems a bit similar to this usecase of showing a popup per feature. Maybe something like that could generalize to any layer-like object?

Alternatively, you could host this change as a custom plugin yourself, and we can link to it in our list of external Folium plugins.

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.

None yet

3 participants