Skip to content
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

Update Map zoom_control variable to also allow string to set position #1884

Merged

Conversation

berrfred
Copy link
Contributor

@berrfred berrfred commented Feb 24, 2024

This is referred to issue #1865

@Conengmo Conengmo self-requested a review February 27, 2024 18:18
Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation is good! I added two comments about the four possible string values. I'd like to make sure users won't make mistakes in these. I hope you can address these comments, afterwards this is good to go!

folium/folium.py Outdated
zoom_control : bool, default True
Display zoom controls on the map.
zoom_control : bool or position string, default True
Display zoom controls on the map, eventually specifying its position.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Display zoom controls on the map, eventually specifying its position.
Display zoom controls on the map. The default `True` places it in the top left corner.
Other options are 'topleft', 'topright', 'bottomleft' or 'bottomright'.

# Zoom control position specified ?
if isinstance(zoom_control, str):
self.zoom_control_position = True
self.zoom_control = zoom_control
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe useful to add a check here for valid values? Now if a users makes an error, they just get a grey screen. For example something like:

Suggested change
self.zoom_control = zoom_control
self.zoom_control = zoom_control
if zoom_control not in {'topleft', 'topright', 'bottomleft', 'bottomright'}:
raise ValueError("Incorrect value for `zoom_control`, choose from 'topleft', 'topright', 'bottomleft' or 'bottomright'.")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had considered introducing a check on valid position values but looking at various plugins I could not find such a check so I thought it was considered as unnecessary ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, this code base grew quite organically over time, so it's not always consistent. Thanks for putting it in!

@Conengmo Conengmo changed the title Updated zoom_control variable to handle either True/False or position Update Map zoom_control variable to also allow string to set position Mar 4, 2024
@Conengmo
Copy link
Member

Conengmo commented Mar 4, 2024

Somethings going on with the tests, it's probably not related to this PR.

@Conengmo Conengmo merged commit b119c13 into python-visualization:main Mar 5, 2024
12 checks passed
@Conengmo
Copy link
Member

Conengmo commented Mar 5, 2024

Thanks for your contribution @berrfred!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants