-
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
Leaflet control #1902
base: main
Are you sure you want to change the base?
Leaflet control #1902
Changes from all commits
4eae936
feb0930
d20567e
d020cc5
c5ce381
1f55089
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,7 +95,7 @@ def __init__( | |
inner_icon_style="", | ||
spin=False, | ||
number=None, | ||
**kwargs | ||
**kwargs, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are all these trailing commas in this PR? Nothing can follow a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They were added by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could switch over to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad, I thought we used ruff as a formatter as well. I can create an extra PR to ditch black if you want, but I have no strong opinion either way. If not I will undo the ruff formatting changes. |
||
): | ||
super().__init__() | ||
self._name = "BeautifyIcon" | ||
|
@@ -111,5 +111,5 @@ def __init__( | |
spin=spin, | ||
isAlphaNumericIcon=number is not None, | ||
text=number, | ||
**kwargs | ||
**kwargs, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,9 @@ | ||
from branca.element import MacroElement | ||
from jinja2 import Template | ||
|
||
from folium.elements import JSCSSMixin | ||
from folium.utilities import parse_options | ||
from folium.features import Control | ||
from folium.utilities import JsCode | ||
|
||
|
||
class MousePosition(JSCSSMixin, MacroElement): | ||
class MousePosition(JSCSSMixin, Control): | ||
"""Add a field that shows the coordinates of the mouse position. | ||
|
||
Uses the Leaflet plugin by Ardhi Lukianto under MIT license. | ||
|
@@ -45,34 +43,6 @@ class MousePosition(JSCSSMixin, MacroElement): | |
|
||
""" | ||
|
||
_template = Template( | ||
""" | ||
{% macro script(this, kwargs) %} | ||
var {{ this.get_name() }} = new L.Control.MousePosition( | ||
{{ this.options|tojson }} | ||
); | ||
{{ this.get_name() }}.options["latFormatter"] = | ||
{{ this.lat_formatter }}; | ||
{{ this.get_name() }}.options["lngFormatter"] = | ||
{{ this.lng_formatter }}; | ||
{{ this._parent.get_name() }}.addControl({{ this.get_name() }}); | ||
{% endmacro %} | ||
""" | ||
) | ||
|
||
default_js = [ | ||
( | ||
"Control_MousePosition_js", | ||
"https://cdn.jsdelivr.net/gh/ardhi/Leaflet.MousePosition/src/L.Control.MousePosition.min.js", | ||
) | ||
] | ||
default_css = [ | ||
( | ||
"Control_MousePosition_css", | ||
"https://cdn.jsdelivr.net/gh/ardhi/Leaflet.MousePosition/src/L.Control.MousePosition.min.css", | ||
) | ||
] | ||
|
||
def __init__( | ||
self, | ||
position="bottomright", | ||
|
@@ -83,19 +53,25 @@ def __init__( | |
prefix="", | ||
lat_formatter=None, | ||
lng_formatter=None, | ||
**kwargs | ||
**kwargs, | ||
): | ||
super().__init__() | ||
self._name = "MousePosition" | ||
|
||
self.options = parse_options( | ||
super().__init__( | ||
control="MousePosition", | ||
position=position, | ||
separator=separator, | ||
empty_string=empty_string, | ||
lng_first=lng_first, | ||
num_digits=num_digits, | ||
prefix=prefix, | ||
**kwargs | ||
lat_formatter=JsCode(lat_formatter) if lat_formatter else None, | ||
lng_formatter=JsCode(lng_formatter) if lng_formatter else None, | ||
**kwargs, | ||
) | ||
self.add_js_link( | ||
"Control_MousePosition_js", | ||
"https://cdn.jsdelivr.net/gh/ardhi/Leaflet.MousePosition/src/L.Control.MousePosition.min.js", | ||
) | ||
self.add_css_link( | ||
"Control_MousePosition_css", | ||
"https://cdn.jsdelivr.net/gh/ardhi/Leaflet.MousePosition/src/L.Control.MousePosition.min.css", | ||
Comment on lines
+70
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? The method calls seem cleaner to me compared to the static attributes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the static attributes allow users that want to create offline maps to bundle these dependencies and then update the values in the static attributes. I know they might also use these new methods, but it's breaking behavior to define these in the init for this use case. Also, I think it's better to have a consistent way of working. So not have two different ways of defining dependencies. |
||
) | ||
self.lat_formatter = lat_formatter or "undefined" | ||
self.lng_formatter = lng_formatter or "undefined" |
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.
how would the
CustomControl
thing we also want use this?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.
Good point and the answer is I don't know yet.
I noted that the
CustomControl
is like aControl
that is linked to a layer, something I did not realize at first. In pureLeaflet
this is something of an anomaly, as bothControl
s andLayer
s are contained in aMap
and there is no structural association betweenControl
andLayer
. Typically it is the other way round and aControl
determines the visibility of theLayer
.My guess is it would be fine as you are implementing it now. It would probably be the same as other
Controls
that are associated with a set ofLayers
.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.
That's interesting... So I guess the custom control is a bit of a weird one, and it's okay not to consider it here.