-
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
Geocoder plugin: support other built-in providers #1852
Geocoder plugin: support other built-in providers #1852
Conversation
…ed by leaflet-control-geocoder
ac95f40
to
f716052
Compare
856e5aa
to
5c7d7a0
Compare
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.
Looks good overall! Few small comments before we can merge this.
folium/plugins/geocoder.py
Outdated
collapsed: bool = False, | ||
position: str = "topright", | ||
add_marker: bool = True, | ||
geocode_zoom: int | None = 11, |
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.
as the default zoom level of 11 is not suitable for close-up locations (and only works when add_marker is False)
Question: is 11 a good default? Or would None be a better default?
And another question: should we check or correct this value when add_marker
is False?
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.
11 would preserve the current behavior so existing users would not be caught by surprise. The real gotcha would be the case when add_marker
=True (when default handler in leaflet-control-geocoder will be using fit_bounds
with no regard to current map zoom). What may not be obvious to normal folium users (with no knowledge of JS) is that both the default event handler for markgeocode
as well as the one defined in _template
would be triggered.
Not sure if there is a need to correct (beyond keeping the current default of 11). Can you explain more?
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.
Not sure if there is a need to correct (beyond keeping the current default of 11).
That's good then. It was more a question for you. We can keep it like it is now then.
Thanks @prusswan! Nice addition! |
…on#1852) * expanding Geocoder plugin to support other built-in providers supported by leaflet-control-geocoder * Added geocode_zoom option for setting zoom level used to display the geocode result * fixed docstring and type notation for geocode_zoom * dropped geocode_ prefix in arguments * fixed import block (ruff --fix)
tested with Nominatim, Photon, Google: