DigitalCommons / mykomap-monolith

A web application for mapping initiatives in the Solidarity Economy
0 stars 0 forks source link

[CWM] Restructure front-end architecture #13

Closed rogup closed 1 month ago

rogup commented 2 months ago

https://github.com/DigitalCommons/mykomap-monolith/blob/main/docs/architecture.md#possible-improvements

We should implement the first two bullet points of the improvements in the above doc, before all the UI gets plumbed in. The improvements are:

There are more details in the architecture doc itself

rogup commented 2 months ago

@wu-lee Please could you remind me of the reason for wanting to use the MapLibre component without a React binding, whilst we use React for the rest of the UI? Was it something about being able to separate the map code from the sidebar panel code in the future?

rogup commented 2 months ago

Looking at the popup methods for MapLibre, I'm wondering if this could be used to use React components for the popups in a way that isn't too messy and keeps fairly clean React and non-React separation https://maplibre.org/maplibre-gl-js/docs/API/classes/Popup/#setdomcontent

wu-lee commented 2 months ago

@wu-lee Please could you remind me of the reason for wanting to use the MapLibre component without a React binding, whilst we use React for the rest of the UI? Was it something about being able to separate the map code from the sidebar panel code in the future?

I was just thinking that our Mykomap components might in the future need to function in the context of a client's pre-existing system, website or app which uses a different framework but that wants to send events/make API calls to Mykomap components to control them or get information back out of them. The specific case was a Drupal website, which would have needed to pan and zoom the map, select pins, etc. in an integrated way, but this is just an example.

Therefore to keep an eye to how we'd do that and not simply assume we're always going to be using React and/or Redux uniformly.

rogup commented 2 months ago

@wu-lee I don't think keeping MapLibre GL separate from the rest of the app really helps us with this. MapLibre doesn't do much by itself; the map is populated and markers are selected according to the app's state, which we are using Redux for. If we want to add API calls, the most sensible way to do this would be for APIs to interact with our Redux store (e.g. send an event to change the filters), which is how our own components alter the UI. Having a clear point of entry like this would stop components becoming entangled and communicating between each other in unexpected ways (which is sort of what happened in the previous codebase).

I think separation would make sense if the map was it's own distinct thing with its own state and didn't need to interact much with the other panels. But the state of the two are logically very connected, so it makes sense for them to share the same store. Modularity for the sake of it would increase the chance of bugs (e.g. UI states becoming inconsistent if API calls between components are misfired), increase complexity & codebase size, and mean that things have to be duplicated (e.g. implementing multiple stores of the same state).

@lin-d-hop Do we know the likelihood of us needing to separate out and export different Mykomap components to other apps in the future?

lin-d-hop commented 1 month ago

Do we know the likelihood of us needing to separate out and export different Mykomap components to other apps in the future?

Seems unlikely to me that we'd be using MM components outside of MM. The most likely situation would be that we'd reuse things in LX. I tend to agree with your reflections @rogup

rogup commented 1 month ago

I've sat on this question about how we use MapLibre GL for a couple of weeks now, and I've landed on a decision.

Although it would be nice to use the fully reactive wrapper that the react-map-gl binding provides, it just adds another dependency that could eventually stop being supported. Our use case is fairly simple so we can implement the functionality of the wrapper ourselves. For the reasons explained in previous comments, we don't want to keep the React and non-React code fully separated since it isn't really possible when we start to add popup code, and it wouldn't be beneficial anyway.

@wu-lee I'm going to start work on this restructuring this afternoon

rogup commented 1 month ago

I've done this restructure and updated the architecture docs