Feature - Support Map handlers when component updated

[jwarykowski]

Overview

Currently, if an end user changes any of the top level handler options in the Map component the associated Leaflet handlers aren't updated when the component updates.

In our own use case, we switch on and off the doubleClickZoom functionality when switching between various modes in our application. I noticed the other handlers were missing so I've included these.

[jwarykowski]

Node 6 build failing on Travis CI with:

error @shellscape/[email protected]: The engine "node" is incompatible with this module. Expected version ">= 7.6.0".
error Found incompatible module

@PaulLeCam - don't know whether you've seen this before or whether its inconsistent, but if you can re-run that would be great.

[PaulLeCam]

Thanks for this PR, good idea!

Could you merge from master please? That should fix the failing Travis build.
Could you also update the documentation regarding the "dynamic properties" of the Map component please?

[PaulLeCam]

Why checking if scrollWheelZoom is a string?

[jwarykowski]

scrollWheelZoom accepts a boolean value or a string in the latest leaflet documentation

[PaulLeCam]

Why checking if touchZoom is a string?

[jwarykowski]

As above - link

[jwarykowski]

@PaulLeCam Updated as discussed. Just to note, I took another look at the Leaflet source code regarding the string value that can be passed for scrollWheelZoom and touchZoom and it appears to only check for the 'center' value so I've made the above conditionals stricter.

[PaulLeCam]

Thanks!
I think it might needs additional logic in the scrollWheelZoom and touchZoom, the behavior when setting the value to center would enable the handler, but as if the value provided was true rather than the expected center behavior, unless I'm missing something?

One way to achieve this could be to update the map.options Object in the Leaflet element to reflect the change, I don't think it needs to check for the center value specifically, checking for the string type is likely a better way to make sure the behavior will also work for other values that could be added by Leaflet later.

[jwarykowski]

Hey @PaulLeCam, I've reset the conditional logic for scrollWheelZoom and touchZoom in the latest commit.

In terms of your first point, I'm not entirely sure what you mean. However, the logic checks whether the changed value is true OR a string type. In terms of the leaflet code, regardless of the value passed (true or center), we just need to enable/disable the handler.

It checks the value passed down when performing an action e.g. scrollWheelZoom's _performZoom

Let me know if you want me to update anything else.

[PaulLeCam]

Thanks for the changes!

Regarding enabling the handler when setting the value to center for scrollWheelZoom and touchZoom, I'm not sure it's enough to enter this condition for example. If I understand it correctly, if the map is created with scrollWheelZoom={false} and then updated to scrollWheelZoom="center", the handler would be enabled but not behaving as expected because map.options.scrollWheelZoom would still have the value false, so I think before enabling the handler, this.leafletElement.options.scrollWheelZoom should be set to the scrollWheelZoom value in updateLeafletElement(), does it make sense?

[jwarykowski]

Hey @PaulLeCam, apologies, now I know what you mean. Thanks for the detailed explanation above. I've updated the functionality in the latest commit.

[jwarykowski]

Hey @PaulLeCam, just checking (no rush) but is there anything else to add here?

[PaulLeCam]

Looks great, thanks!
Sorry I don't have much time to play with the branch at the moment but I'll try to get to it in the coming days.