mariusandra / pigeon-maps

ReactJS Maps without external dependencies
https://pigeon-maps.js.org/
MIT License
3.44k stars 142 forks source link

Attach sub elements #150

Closed baldulin closed 2 years ago

baldulin commented 2 years ago

Right now it's not that easy to wrap components around each other and still attach them to the map like all the other components will (hopefully I'm not missing something).

This pr makes the cloning part of the renderOverlay method accessible via a prop (I call that cloning part attaching to the map). This would enable all components that are attached to the map already to attach other components as well cleanly.

If this pr is accepted I would create another one where the PigeonProps are stored in a context. Thus a component could attach itself as a child component even inside of fragments or whatever without the intervention of the parent.

mariusandra commented 2 years ago

Sorry, but going to have to pass on this. Using the React context would be the right call here, so why not just do that?

I'd be in favour of just having one React context that exposes functions like:

interface PigeonContext { 
  getMapState: () => MapState
  getMapProps: () => MapProps
  latLngToPixel: (latLng: Point, center?: Point, zoom?: number) => Point
  pixelToLatLng: (pixel: Point, center?: Point, zoom?: number) => Point
  setCenterZoom: (center: Point | null, zoom: number, zoomAround?: Point | null, animationDuration?: number) => void
}

It's best to expose mapState and mapProps via functions like these, and not to change the context's value on every render... that'll get slow.

baldulin commented 2 years ago

I think I was a bit unclear, the reasoning I implemented this was basically I just wrote a simple clustering component like here. And as you pointed out to "attach" the child nodes one needs to use a function exactly like Map does to attach overlays in general. If a component would have access to the original version of that function, than one is assured the wrapping components works exactly as it would work for Map as well. The other benefit would have been that one could write this function into a context and components could "attach" themselves, this would however not work in conjunction with wrapping. So I guess it's fine not to merge this, I'll be able to implement a Cluster component without that function anyway.

A note to the speed issue though, I'm not sure If I understand correctly but if a component needs to use latLngToPixel to render itself correctly inside the map, than that component is implicitly dependent on mapState, this means that it must be rerendered if mapState changes, however if mapState is not part of PigeonContext than it might not be rerendered at all. If I'm not mistaken here, latLngToPixel and all the other functions in the context have to be pure functions. And I suppose this means for PigeonContext to be useful at all it must change nearly on every render, at least everytime mapState changes. And yea that will be slow if one uses it. I guess that's the reason I'm not using leaflet anymore. So as a conclusion I'm not gonna write a pr for a context.