-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fix icon when passing Marker to GeoJson #2086
Fix icon when passing Marker to GeoJson #2086
Conversation
folium/map.py
Outdated
@@ -403,8 +403,11 @@ def __init__( | |||
self.options = remove_empty( | |||
draggable=draggable or None, autoPan=draggable or None, **kwargs | |||
) | |||
# this attribute is not used by Marker, but by GeoJson | |||
self.icon: Optional[Icon] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still fails if people use the add_child
method to add an icon. I am okay with that, but I think we should be aware of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the render
method (roughly lines 430-436) was originally rewritten to allow people to use add_child
. If we go back to icon as a member variable, we should make it consistent throughout the class.
Is this waiting for some changes or are we fine with it as is? I am unable to properly see the consequences here so will let you be the judges but I'd love this to be merged to fix geopandas CI. |
Ah, sorry it escaped my attention. Will look at it presently. |
ee52330
to
df180b3
Compare
Fixes #2084, which is an issue introduced by #2068.
We shouldn't have removed
self.icon
inMarker
. It's not directly visible where it is used, so I added a comment to prevent this from happening again.