Semantics of IControl.onAdd/onRemove are inconsistent

mapbox / mapbox-gl-js

Interactive, thoroughly customizable maps in the browser, powered by vector tiles and WebGL

https://docs.mapbox.com/mapbox-gl-js/

Other

11.17k stars 2.22k forks source link

Open stevage opened 1 year ago

stevage commented 1 year ago

Semantics of onAdd: expected to create and return a DOM element which Map will add within its own div.

Semantics of onRemove: expected to remove its own DOM element.

Surprised this inconsistency hasn't been flagged before.

It seems to me that map.removeControl(X) should remove X, rather than relying on X to remove itself.

mourner commented 1 year ago

Yes, perhaps it's inconsistent and an oversight, but I'm not sure whether it's worth changing it:

map.removeControl would somehow need to know which element to remove. This means we would have to keep track of the element returned from onAdd before in a lookup table, complicating implementation, or expose an additional required API method for IControl to get the element of a control. Which option would you prefer?
It might be a breaking change for all existing control plugins.