Don't enable by default and don't use the map options

[boldtrn]

I really like this plugin. It seems to be offering exactly what I was looking for.

But I don't like the way the plugin gets activated once it is loaded. Also I think it's not ideal that we use the map options to configure the plugin.

Wouldn't it be better to add this plugin on demand with it's own config options like:

L.Control.SleepMapControl({ options }).addTo(map);

Since using this plugin is only a minor side-track for my code, I have to add special handling like this:

mapOptions = {...};

if(...){
   require('leaflet-sleep');
   mapOptions.sleepOpacity = X;
   ...
}

map = L.map('map', mapOptions);

Most other Leaflet plugins seem to use their own construction as well and don't use the map for that.

[atstp]

But I don't like the way the plugin gets activated once it is loaded.

I understand. It was what was needed when I wrote the plugin, but making the behavior opt-in would probably be favorable.

Also I think it's not ideal that we use the map options to configure the plugin.

I'm okay with this too. I have a slight preference towards configuring the map, but I'm not attached to it.

Wouldn't it be better to add this plugin on demand with it's own config options like:

L.Control.SleepMapControl({ options }).addTo(map);

If i were to reimplement leaflet-sleep right now, I think I'd stick all the sleep behavior/logic in a Handler so a user can .enable() and .disable() it. Then, an instance of Control could handle the UI.

Once again though, If you put in the work and sent a PR, I'd be happy to put my personal preferences aside.

---

For your issue with the defaults, L.Class provides .mergeOptions which should help your situation.

something like:

require('leaflet-sleep');
L.Map.mergeOptions({ /* desired defaults for all maps */ });
// now all maps will use the newly merged options

should make things easier (if i'm understanding your issue).

[boldtrn]

Thanks for the detailed feedback.

If i were to reimplement leaflet-sleep right now, I think I'd stick all the sleep behavior/logic in a Handler so a user can .enable() and .disable() it. Then, an instance of Control could handle the UI.

Yes, doing this through a Handler certainly makes sense. For me this was more about an example how an opt-in could look like.

My biggest issue is that calling require('leaflet-sleep'); automatically registers the handler. Since I only use this for a minor feature on my website that targets <5% of users. The best would be if it could be added after initializing the map, to be a bit more flexible.

That said, my current solution seems to be working fine. But it felt a bit overly complicated to work around it. So it's not a blocker for me, but maybe this could be considered with the next bigger update or potential rewrite, if there is any :).

[atstp]

Sounds good.

If this plugin ever needs/gets breaking changes, I'll try to fit these two in with a 1.x release:

• maps by default retain standard leaflet behavior (no sleeping)
• the UI is configured with an L.Control instance

[mrubli]

In case anyone's interested, I ended up first forking this project and later rewriting large parts of it to make it fit more nicely with the Leaflet L.Control API:
https://gitlab.com/mrubli/leaflet-freezy