GeoJson Popup #1023
GeoJson Popup #1023
Conversation
There was a problem hiding this comment.
Cool Jason! This is going in the right direction: having a base class that we reuse for the popups and the tooltips.
Overall I don't have much comments, apart from treating the Vega stuff separately and how the class_name things work.
When we agree on the implementation it could use some code styling but that'll come then.
| self.aliases = aliases if aliases is not None else fields | ||
| self.labels = labels | ||
| self.localize = localize | ||
| self.class_name = class_name |
There was a problem hiding this comment.
I don't really understand how class_name is used. Why not use self.get_name()? I see how it's passed as option to initializing the Leaflet objects, and how it is used as CSS selector in style, but I don't get why.
There was a problem hiding this comment.
@Conengmo - sorry for the extended delay on the modifications to this PR - I think I've addressed most of your concerns with this revision.
re: the class_name: I thought it might be useful for users wanting to add custom css declarations to style the tooltips with a human-readable name. It's also allowed us to take the inline styling out of the html elements and add a top level styling to the class in the head of the document.
folium/features.py
Outdated
| u""",{{ this.popup_options | tojson | safe }}) | ||
| {% endmacro %}""") | ||
|
|
||
| def __init__(self, fields=None, aliases=None, labels=True, style="margin: auto;", vegalite=None, map_key=None, |
There was a problem hiding this comment.
What do you think about giving this popup the same options as the 'regular' Popup class? I mean show and sticky options.
folium/features.py
Outdated
| u""",{{ this.popup_options | tojson | safe }}) | ||
| {% endmacro %}""") | ||
|
|
||
| def __init__(self, fields=None, aliases=None, labels=True, style="margin: auto;", vegalite=None, map_key=None, |
There was a problem hiding this comment.
The default argument for style is a bit confusing. Is it necessary?
ocefpaf
force-pushed
the
master
branch
3 times, most recently
from
53546b8 to
9f2299a
Compare
|
@Conengmo , @jtbaker could we merge this feature into master? Or there is still missing something? All test checks are now passed. In my opinion, having a tooltip as the only way, to add extra information to a marker is a big limitation of folium geojson. When loading a geojson file from local drive is still possible loop through all points and create a normal marker with a popup, with is ok but doing extra work. However, if the data source is an url, there is no way to generate a popup for each marker. Also tooltips aren't available on mobile devices. |
Both GeoJsonPopup and GeoJsonDetail inherit from this class as they share common attributes. Also updated the GeoJson JS template to reference this Tooltip object, and added a test to make sure that the values passed exist in the properties. Merge upstream.
folium/features.py
Outdated
| self.style = style | ||
| class GeoJsonPopup(GeoJsonDetail): | ||
| """ | ||
| Create a popup feature to bind to each element of a GeoJson layer based on |
|
Nice Jason, I'll look at this PR again. |
|
Thanks for the follow up. I was just unsure what the status was as far as merging. Thank you for clarifying! |
Added GeoJsonPopup class that
combinesextends functionality of GeoJsonTooltipand VegaLite existing functionality by database-style 'joining' key values from GeoJson to properties in the VegaLite chart data section.