keithamoss / mapa

2 stars 0 forks source link

Consider rearchitecting to core App and OLMap code #1299

Open keithamoss opened 2 months ago

keithamoss commented 2 months ago

The complexity has grown immensely since the original implementation. We've added many new map controls, we've added requestAnimationFrame for headings, we've added the (largely unused) WebGLPointsLayer implementation, we've added Map Switcher, we've changed how we load the Icons Library, et cetera.

Seems about time for a rethink and a review of current best practice and other approaches for using OL and React together.

We have a few pain points and odd bugs we've come across that have been hard to track down. Nor am I happy with how OLMap handles layer switching when features change. Likewise, our large use of Refs and passing those to OL event handlers. Why not just use the map we get with each event object and attach any extra metadata on to the map/layers with OL's general metadata/properties implmentation?

It all feels like a bit of a house of cards and worthwhile starting from scratch. I'd suggest with OLMap.

We could also consider redoing certain parts, like having the layers created before the map, and added to the map at the start. Not sure why it's the way it is now. We already just dump + reload features into the layer when they change.

The main App state also has more loading states that get shown to the user now - the initial loading of JS assets, the loading of the icons library, and the (re)loading of features.

Ref for inspo: https://mxd.codes/articles/how-to-create-a-web-map-with-open-layers-and-react

keithamoss commented 2 months ago

This would also be a good chance to review some aspects of React best practice. E.g. the suggestion I read on SO that Refs should only be mutated in useEffect et cetera and not during rendering. At the moment, we're setting almost all of our Refs .current on each render of OLMap.