Zoom to Overlay Selection

[galewis2]

WIP.

Just to track what I'm working on. This is related to #1313

• Move to a plugin
• Better detection of getBounds availability than the try/catch
• Avoid on-the-fly FeatureGroup creation
• Make optional via a parameter (on LayerControl or Map?)
• Make sure I meet the https://github.com/python-visualization/folium/blob/master/.github/CONTRIBUTING.md requirements of documentation, test passing, etc

[Conengmo]

Hi @galewis2 , I know this is still a draft but I already wanted to give some comments and questions, help develop this.

I like the idea of detecting whether a layer has the getBounds method and using it in that case. That way we can apply this implicitly on all classes that support it, smart.

I'm not sure about adding a FeatureGroup on the fly, it seems unrobust. We already have a list of layers in {{ this.get_name() }}.base_layers and {{ this.get_name() }}.overlays. You could add on('overlayadd to those?

I don't know if we need to do anything on overlayremove. If a user deselects a layer, should the bounds of the map change? My initial guess would be no.

We can discuss this of course. I'm looking forward to your next changes on this.

By the way don't worry about tests for this. When it's finished we can add an example to an existing or new Jupyter notebook. We'll test those.

[galewis2]

Hi @galewis2 , I know this is still a draft but I already wanted to give some comments and questions, help develop this.

I like the idea of detecting whether a layer has the getBounds method and using it in that case. That way we can apply this implicitly on all classes that support it, smart.

I'm not sure about adding a FeatureGroup on the fly, it seems unrobust. We already have a list of layers in {{ this.get_name() }}.base_layers and {{ this.get_name() }}.overlays. You could add on('overlayadd to those?

No problem, that was just a quick hack that I forgot to add to the todo list. I wanted a quick method whereby I got the bounds of all layers in one call without doing any sort of max/min bbox calculations

I don't know if we need to do anything on overlayremove. If a user deselects a layer, should the bounds of the map change? My initial guess would be no.

I guess this is a user expectation and use-case question. Should the map on layer (de)select be constrained to those layers?

We can discuss this of course. I'm looking forward to your next changes on this.

By the way don't worry about tests for this. When it's finished we can add an example to an existing or new Jupyter notebook. We'll test those.

[galewis2]

I'm not sure about adding a FeatureGroup on the fly, it seems unrobust. We already have a list of layers in {{ this.get_name() }}.base_layers and {{ this.get_name() }}.overlays. You could add on('overlayadd to those?

From what I can tell, base_layers and overlays are for generating the LayerControl object and don't reflect what's currently selected. I'm not sure what you mean here?

I'm going to switch from an on-the-fly FeatureGroup to a LatLngBounds objects and do extend on that. Same outcome, but simpler object and more refined for how it's being used.

[galewis2]

@Conengmo Ready for further discussion about implementation.

[Conengmo]

I just tested your current implementation and it works great. I know I said this before but I like how this works for any layer with getBounds. Also good how you got past the on-the-fly featuregroup thing. I think I had a different idea about this feature when I wrote my previous comments, so we may have had some confusion there. But I'm happy with how it looks now. I don't have any major comments on the implementation.

Some more detailed questions/comments:

• Have you considered adding padding?
• Since the two event handlers are basically the same, can we combine them? Note that jQuery is currently a requirement so you're free to use it.
• Please use snake_case for arguments. The zoom_selected variable shouldn't be added to self.options because it's being passed to L.control.layers. Instead store it on self and retrieve it from this in the template.
• Have you considered using flyToBounds? I like how it gives a sense of where the map is moving. But I wonder if it gets annoying.
• Have you thought about whether to also apply this on map load?

I was looking in what example notebook we can showcase this. I don't think it fits in any existing one. Maybe create a new one? I'm working on testing the notebooks in our test suite, so that will be the test case for this feature.

[galewis2]

I just tested your current implementation and it works great. I know I said this before but I like how this works for any layer with getBounds. Also good how you got past the on-the-fly featuregroup thing. I think I had a different idea about this feature when I wrote my previous comments, so we may have had some confusion there. But I'm happy with how it looks now. I don't have any major comments on the implementation.

Some more detailed questions/comments:

* Have you considered adding padding?

I'm happy with the default padding. The only issue I can see is that markers may be cutoff when at the top of the map, but I decided to leave it as is as they have a static size in the browser so setting a padding may be tricky.

* Since the two event handlers are basically the same, can we combine them? Note that jQuery is currently a requirement so you're free to use it.

Done.

* Please use snake_case for arguments. The `zoom_selected` variable shouldn't be added to `self.options` because it's being passed to `L.control.layers`. Instead store it on `self` and retrieve it from `this` in the template.

Done.

* Have you considered using `flyToBounds`? I like how it gives a sense of where the map is moving. But I wonder if it gets annoying.

Seems okay to me, done.

* Have you thought about whether to also apply this on map load?

I attempted to do this now, but I think I'm running into the problem where firing a flyto on load for the map object wasn't working as the layers weren't initially added to the map. Which also requires the layers to be defined in the map object. At this stage I think it would be a pain to implement when there's a workaround of getting all the layerbounds in python and applying that manually on map instantiation

I was looking in what example notebook we can showcase this. I don't think it fits in any existing one. Maybe create a new one? I'm working on testing the notebooks in our test suite, so that will be the test case for this feature.

[Conengmo]

Code looks good! We're very close. Three comments:

This doesn't work with all types of layers, for example Heatmap. I'm okay with that. But maybe good to add a line to the docstring that warns users it may not work for all layer types.

One thing I tried to get it to apply this on map load was:

function customFlyToBounds () {
    [...your code that calculates bounds and applies fly to///]
}
{{ this.parent }}.on('addlayer removelayer', customFlyToBounds);
customFlyToBounds();

This works because LayerControl is added to the map after the layers it controls are. So at the point where we call this function the layers must already have been added to the map.

What do you think of this?

Now the only thing that remains is add a test case. Please rebase or merge master into your branch. Then add a new notebook, I'd suggest examples/LayerControl.ipynb. Add a simple example that showcases this functionality. The notebook will be the documentation and test case.

[galewis2]

Code looks good! We're very close. Three comments:

This doesn't work with all types of layers, for example Heatmap. I'm okay with that. But maybe good to add a line to the docstring that warns users it may not work for all layer types.

One thing I tried to get it to apply this on map load was:

function customFlyToBounds () {
    [...your code that calculates bounds and applies fly to///]
}
{{ this.parent }}.on('addlayer removelayer', customFlyToBounds);
customFlyToBounds();

This works because LayerControl is added to the map after the layers it controls are. So at the point where we call this function the layers must already have been added to the map.

What do you think of this?

Now the only thing that remains is add a test case. Please rebase or merge master into your branch. Then add a new notebook, I'd suggest examples/LayerControl.ipynb. Add a simple example that showcases this functionality. The notebook will be the documentation and test case.

Sounds good, hopefully I will get some time to look at it this week!