search

DigitalCommons / land-explorer-front-end

React app for the Land Explorer front end
http://landexplorer.cc
GNU Affero General Public License v3.0
1 stars 0 forks source link

Edit Marker to enable moving a marker location #317

Open lin-d-hop opened 3 months ago

lin-d-hop commented 3 months ago

Describe the bug In the Drawing Tools menu of LX the Edit option allows the user to edit the points in a line or a poly. However when you click an existing marker you can't do anything. It would make sense to also be able to edit the location of a marker.

To Reproduce Steps to reproduce the behavior:

  1. Ensure there is an existing drawn marker on your map.
  2. Go to Drawing Tools
  3. Click on Edit
  4. Go to an existing marker and click on it
  5. You can't change the location. Note - this behaviour is different to Lines or Polys so feels like a bug.

Expected behaviour When you click on a marker after clicking edit you should be able to drag the marker to a new location

lin-d-hop commented 2 months ago

After some preliminary investigations... Mapbox gl doesn't allow markers to be moved. There is a chance we could implement markers via the mapbox gl 'feature' functionality, however this might break our clustering implementation

Quick spike to see what happens. If anything breaks, let's update the issue here and close it.

ms0ur1s commented 2 months ago

Getting this at the moment:

Image

@rogup is the below code sufficient for a quick test? The correct way, I guess, would be to create a bespoke feature from scratch more akin to our polygon and lines.

Image

ms0ur1s commented 2 months ago

@lin-d-hop, I'll wait to see what @rogup says about the above before closing.

rogup commented 2 months ago

@ms0ur1s I think that error is because an expected prop hasn't been given, so I don't know if it's using Features that is necessarily the problem.

I had another quick look at how polygons and lines are editable and can see that we use the react-mapbox-gl-draw drawControl component. So I think we don't actually need to implement our own onDragStart/End methods, and could do the same for markers.

I think it does seem doable to display markers in this way, as a GeoJSON layer of Point features, and then feed in the features to drawControl. This would make how we display markers, polys and lines more consistent. I found this library too, which I did a really quick test with, and it looks like clustering will still work https://www.npmjs.com/package/react-mapbox-gl-cluster

The branch I played with: https://github.com/DigitalCommons/land-explorer-front-end/commit/09d082d88054d2e2e492c3ad533e0ae1d1285d67

So I think making markers moveable is possible, but wouldn't be a really quick fix (it would probably take 1-2 days)

ms0ur1s commented 2 months ago

@rogup, thanks man. Funnily enough, when I was doing a little digging in the react-mapbox-gl documentation I had a similar though about the GeoJSON layer of Point features. The cluster library is a great find.

If @lin-d-hop is happy to go ahead, do you think these changes are within my capabilities, if I use your initial work as a starting point and make sure give you a shout when I encounter any difficulties?

rogup commented 2 months ago

Yeah I think it's something that you can do :) And learning about how stuff is rendered in Mapbox is probably a useful thing, since it relates to your frontend UI expertise and might be related to work we do soon on Mykomap too.

Happy to answer any questions or jump on a call to give any guidance. The structure of data in GeoJSON and the way that feature IDs work is something that I have been working on a lot recently (with property boundaries)

ms0ur1s commented 2 months ago

Thanks @rogup for all your help and patience, as usual. 😀

@lin-d-hop, in light of Rohit's comments are you happy for me to carry on with this as a dev ticket?

lin-d-hop commented 2 months ago

@ms0ur1s Short answer - sure go for it Long answer - 1-2 days is pretty significant for this, particularly as I expect Marcel's actual time vs Rohit's estimate won't actually be the same. I would say park it, but:

  1. Since we are trying to clear the decks for MM stuff small tasks are useful in the interim
  2. Hopefully relevant for MM too, and good to get across this code

So yeah, progress on the caveat that this is low priority and if other things come in in the meantime we will be parking it.

ms0ur1s commented 2 months ago

Thanks @lin-d-hop, understood. Also, if I get any sense that I'm hitting a brick wall I reach out to @rogup.

lin-d-hop commented 2 months ago

Cool, yes, rule of thumb - don't be stuck for an hour. Reach out!