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

Geojson zoom on click fixes #1349

Merged

Conversation

Conengmo
Copy link
Member

@Conengmo Conengmo commented Jun 5, 2020

Close #1345 and close #1162

Apply two changes to the GeoJson class:

Don't zoom on click if the geometry type doesn't support it. This is the case for point geometries.

Add a parameter to control the zoom on click functionality. Disable it by default. I thought about this and I think it's best not to hijack the zoom by default. Users who do want that behavior can enable it as follows:

GeoJson(zoom_on_click=True)

or

choropleth = Choropleth()
choropleth.geojson.zoom_on_click = True

Copy link
Contributor

@jtbaker jtbaker left a comment

Choose a reason for hiding this comment

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

This looks great to me @Conengmo. I agree on the default non-zoom-on-click behavior, (although it did feel pretty cool to get that kind of interaction out of the box when first getting started with the library). As a part of this PR, maybe a good thing would be to add the parameter usage to some of the notebook examples so that it's obvious to people when getting started with the library. I'm adding a few ideas as tweaks inline.

@Conengmo
Copy link
Member Author

Conengmo commented Jun 5, 2020

Thanks for the review Jason!

maybe a good thing would be to add the parameter usage to some of the notebook examples

you're totally right, I'll add that

{{ this.parent_map.get_name() }}.fitBounds(e.target.getBounds());
if (typeof e.target.getBounds === 'function') {
{{ this.parent_map.get_name() }}.fitBounds(e.target.getBounds());
}
Copy link
Contributor

@jtbaker jtbaker Jun 5, 2020

Choose a reason for hiding this comment

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

Here, how about to handle zooming for point geometries as well? I'm not sure about best generalized options for the .fitBounds() method, it supports both padding and maxZoom options.

else {
    const targetLatLng = e.target.getLatLng()
    const bounds = L.latLngBounds([targetLatLng, targetLatLng])
    {{ this.parent_map.get_name() }}.fitBounds(bounds, {padding: [5,5], maxZoom: 8});
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I think your code is fine but I'd rather not introduce more fragility here. This may work for point geometries but I'm unsure what new bugs it might introduce. I'll just merge the PR as is, let's consider any new functionality separately.

@Conengmo Conengmo merged commit 86319bb into python-visualization:master Jun 7, 2020
@Conengmo Conengmo deleted the geojson-zoom-on-click branch June 7, 2020 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants