-
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 1 commit
4eae936
feb0930
d20567e
d020cc5
c5ce381
1f55089
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Most of the code of mouse_position has been removed. Instead it simply calls the constructor of Control with the typed parameters.
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,6 @@ | ||
from jinja2 import Template | ||
|
||
from folium.elements import JSCSSMixin | ||
from folium.features import Control | ||
from folium.utilities import parse_options | ||
from folium.utilities import JsCode | ||
|
||
|
||
class MousePosition(JSCSSMixin, Control): | ||
|
@@ -45,34 +43,6 @@ class MousePosition(JSCSSMixin, Control): | |
|
||
""" | ||
|
||
_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.
why are all these trailing commas in this PR? Nothing can follow a
**kwargs
entry so a trailing comma is not necessary. Black or ruff doesn't add these if I'm not mistaken, so why add them?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.
They were added by
ruff format
, so don't blame me :-). I agree they do not make sense in this case. As far as I know there is no option to remove these trailing commas. I can manually undo these kind of changes, but its a hassle. I'd prefer to just blindly followruff
.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.
We don't run
ruff format
on this repo yet though. We use Black to autoformat and have Ruff as linter. So it's better not to runruff format
, since it will create a lot of changes.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.
We could switch over to
ruff format
though and ditch Black. Maybe that's good to do anyway, and if we merge that before this PR that also solves the issue.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.
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.