ebrelsford / Leaflet.loading

A simple loading control for Leaflet
MIT License
143 stars 49 forks source link

support LayerGroup #33

Closed glathoud closed 8 years ago

glathoud commented 8 years ago

Context: using a L.control.layers instance and a LayerGroup.

Use case:

var map = L.Map( 'map', { ... , layers : [ some_tilelayer ], loadingControl : true } );
var control = L.control.layers( { a : some_tilelayer, b : some_layergroup } );
control.addTo( map );

when the user clicks on the layer control, and switches to b, the loading indicator appears (correct) but stays forever (bug).

The issue seems to be caused by:

map.on({
  baselayerchange: this._handleLoading,

so that it is expected that the baselayer some_layergroup also fires "load" which never happens.

Tentative fixes

I tried this modification:

_handleLoading: function(e) {
  if (!(e.layer  &&  e.layer instanceof L.LayerGroup))  // <<< inserted this line
    this.addLoader(this.getEventId(e));
},

...but this revives issue 27.

I also tried to protect the layer.on(...) calls, but to no avail.

Current solution

Use a FeatureGroup that is extended to support the "loading" and "load" events: Leaflet.FeatureGroup.LoadEvents.

Remark

I honestly don't know whether this issue is fixable within Leaflet.loading, especially from a design point of view, or whether the issue must be fixed outside of it, e.g. with the Leaflet.FeatureGroup.LoadEvents plugin.

ebrelsford commented 8 years ago

Thanks for this @glathoud. I feel like our current solution to #27 is a bit of a hack currently, so I would prefer to fix that. That is, I would prefer not to call _handleLoading on baselayerchange as we do now. I assumed that would cause problems but haven't run into them myself.

Can you provide a minimal example of this going wrong (eg via jsfiddle or jsbin)?

glathoud commented 8 years ago

Sure: http://jsbin.com/sikageyiva/edit?html,output

Just click on the control on the top-right of the map and switch to "layerGroup".

ebrelsford commented 8 years ago

Thanks @glathoud. I think 21a3e85c should cover it, mind giving it a try?

glathoud commented 8 years ago

Thanks @ebrelsford the fix is working fine.

Detail: the fix could be recursive.

ebrelsford commented 8 years ago

Fair enough. Fixed, let me know if there are any other issues.

glathoud commented 8 years ago

It's running fine at the moment, thanks.