activist-org / activist

An open-source activism platform
https://activist.org
GNU Affero General Public License v3.0
244 stars 196 forks source link

Rework MediaMap component #332

Closed andrewtavis closed 9 months ago

andrewtavis commented 1 year ago

Terms

Description

The current way that the MediaMap component is defined needs to be reworked. Specifically we cannot inject information from the backend into a map pin tooltip using the current method.

Contribution

This has been a known issue for some time, so making this to factor it into the work given the importance of this component.

@momanyisamuel is also knowledgeable of the shortcomings of the current approach. Let’s first discuss how best to implement and then someone can be assigned :)

luuu-xu commented 1 year ago

@andrewtavis @momanyisamuel Why can't we inject backend data into this component? As I am understanding, the component can take an array of strings addresses as props, which comes from event about page where MediaMap is rendered with the props, then when MediaMap is mounted, fetch osmURL(address) to make an array of Marker where we can then display the lat, lon, address for the addresses from an event.

momanyisamuel commented 1 year ago

@luuu-xu this component was implemented with leafletjs library, and at the time we had a discussion with @andrewtavis about using a library that could offer more options since there was a need to use the MediaMap component in more than one place. To give more context the MediaMap component currently takes only one address from the marker object and does not use the array of addresses passed to it as props. thus the need to rework this component to accept dynamic data. The nuxt-leaflet library seemed out of date and we had a consideration of implenting this using vue3-openlayers

andrewtavis commented 1 year ago

Thanks for writing in, @momanyisamuel! I was making this issue and was like β€œI remember there’s a problem and we had talked about fixing thissss πŸ€”πŸ˜…β€

luuu-xu commented 1 year ago

@momanyisamuel I see! Thanks for the input. So I am understanding that the goal is to have the map to render multiple Markers, right? (because now it can only fetch and render 1 Marker, correct me if I'm wrong).

andrewtavis commented 1 year ago

Yes, @luuu-xu :) This was a major drawback for the current implementation. For now we’re ok with one marker for an event page, but for a map view for search we need more markers.

luuu-xu commented 1 year ago

Great! Let me give it a spin! I will try converting it to use vue3-openlayers.

andrewtavis commented 1 year ago

Sounds good, @luuu-xu! Assigning you now :)

luuu-xu commented 1 year ago

@andrewtavis Okay, I have tried several times and searched for solutions but I am stuck now.

Following the vue3-openlayers docs, I installed required dependencies, and got docker up and running.

Then I created a MediaMapOL component which follows this example, ol-map.

But whenever I use this component in our app, I will get this error:

nuxt_frontend   | ✘ [ERROR] Could not resolve "ol/source/Stamen"
nuxt_frontend   | 
nuxt_frontend   |     node_modules/vue3-openlayers/dist/vue3-openlayers.es.js:45:19:
nuxt_frontend   |       45 β”‚ import Stamen from "ol/source/Stamen";
nuxt_frontend   |          β•΅                    ~~~~~~~~~~~~~~~~~~
nuxt_frontend   | 
nuxt_frontend   |   You can mark the path "ol/source/Stamen" as external to exclude it from the bundle, which will remove this error.
nuxt_frontend   | 
nuxt_frontend   | 
nuxt_frontend   | 
nuxt_frontend   |  ERROR  error while updating dependencies:
nuxt_frontend   | Error: Build failed with 1 error:
nuxt_frontend   | node_modules/vue3-openlayers/dist/vue3-openlayers.es.js:45:19: ERROR: Could not resolve "ol/source/Stamen"
nuxt_frontend   |     at failureErrorWithLog (/app/node_modules/esbuild/lib/main.js:1649:15)
nuxt_frontend   |     at /app/node_modules/esbuild/lib/main.js:1058:25
nuxt_frontend   |     at /app/node_modules/esbuild/lib/main.js:1525:9
nuxt_frontend   |     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Inspecting files, ol/source/Stamen is indeed there, and my searches do not give me a working solution.

Please give it a try yourself and let me know if you have the same issue! Cheers!

momanyisamuel commented 1 year ago

Hi @luuu-xu could you try rebuilding the image for the frontend it could be you added it to the local repo on your machine but its not yet installed on the docker instance???

andrewtavis commented 1 year ago

I'll check in on this once we're done with #215, @luuu-xu :) :)

luuu-xu commented 1 year ago

try rebuilding the image for the frontend

@momanyisamuel Yes, I tried running docker-compose up --build to rebuild the image and the build failed because of the error message I posted above.

I also tried installing the required packages on my local machine and run npm run dev to run the frontend, but whenever I am trying to use ol-map, it will break because of the error [ERROR] Could not resolve "ol/source/Stamen".

I would really love you guys to give it a try and let me know if it works on your end, and possibly where I am doing it wrong...

andrewtavis commented 1 year ago

Hey there @luuu-xu πŸ‘‹πŸ˜ŠπŸ˜Š I hope the vacation was a nice time!

Sorry for taking my time in checking this. With you being away I decided to focus on other things, and I sadly was out sick for a bit and have been playing catch up. With this being said... πŸ₯πŸ₯πŸ₯

Seems to be working on my end πŸ™ƒ

Did some digging and these are my steps:

import { defineNuxtPlugin } from "#app";
import Openlayers from "vue3-openlayers";

export default defineNuxtPlugin((nuxtApp) => {
  nuxtApp.vueApp.use(Openlayers);
});

Screenshot_2023-10-19 01 10 38

Let me know if I should do some basic styling and send this along! We can then do some planning and figure out how to have it support all the functionality we need it to have. Would maintain the code for the current component commented out below so we could use it as a reference in the rewrite 😊

Alsooo, lots of progress! I'm really looking forward to getting your feedback on the dev setup as well as all the new features and code quality improvements. Could definitely use your help making sure that certain elements on the page are tab accessible πŸ˜‡

andrewtavis commented 11 months ago

Hey @luuu-xu πŸ‘‹ Checking in to see if you still have interest in working on this issue 😊 Let me know if you'd like to get to it still, or if not I'd be happy to work on this!

As always happy to do a call if you have time. There's been lots of changes since your last commits, and I'd be more than willing to walk you through them!

Hope you're well!

andrewtavis commented 9 months ago

Took a bit of a turn on this and decided to go with MapLibre directly rather than using open layers. As of now there's no solid Vue library for it, but then just using the base TS library is so easy 🀩 Really is a joy. There's currently a bug where the bottom of the map sometimes gets cut off - almost certainly solvable - and as of now basically all functionality has been replaced except for the popup of the event (and more!). Adding extra markers is just duplicating the line that creates the one I have, and beyond that we now have full screen, location services if the user oks it, and we can select points to see a route! More functionality can be added later on 😊 We can talk about all this in the next sync πŸ§‘β€πŸ’»β™»οΈ

andrewtavis commented 9 months ago

To close this issue we just need to add in the tooltip for the event :) Should be some work, but I'll get to it soonish.

andrewtavis commented 9 months ago

Closing this issue as the rest of the functionality has been put into separate issues - namely #679 and #680, with further features being added in other issues that were recently made 😊 Thanks all for the conversation and the research here! Really has turned out nicely πŸ—ΊοΈπŸ™Œ