Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show other markers on the map in the edit mode #581

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

circus2271
Copy link
Contributor

@circus2271 circus2271 commented Mar 2, 2023

@circus2271 circus2271 marked this pull request as draft March 2, 2023 13:51
@circus2271 circus2271 force-pushed the feature/Show-other-markers-on-the-map-in-the-edit-mode branch from bac84b2 to 03633f4 Compare March 2, 2023 14:15
@circus2271 circus2271 marked this pull request as ready for review March 2, 2023 14:19
@circus2271 circus2271 marked this pull request as draft March 2, 2023 14:30
@kabalin
Copy link
Member

kabalin commented Mar 2, 2023

Thanks @circus2271, this needs to be improved:

  1. When I edit existing point and move pin to the different location, the original point remains in place:
    image
    The current point needs to be excluded from the list of markers.
  2. Other displayed points are clickable, it makes very easy to navigate away from editing mode by accidental click on any other point, not sure how critical it is, but I think it is better not to process other point click event while photo is in editing mode (pinging @paul-k-pastvu for opinion).

@circus2271
Copy link
Contributor Author

@kabalin take a look at 'another draft' commit, please. Is it terribly wrong?

@circus2271 circus2271 force-pushed the feature/Show-other-markers-on-the-map-in-the-edit-mode branch from 7dd656d to 5090950 Compare March 4, 2023 08:53
@circus2271 circus2271 marked this pull request as ready for review March 4, 2023 08:56
@circus2271 circus2271 marked this pull request as draft March 4, 2023 09:58
@circus2271 circus2271 force-pushed the feature/Show-other-markers-on-the-map-in-the-edit-mode branch 2 times, most recently from 8cb88f0 to 33c10df Compare March 4, 2023 10:05
@circus2271 circus2271 marked this pull request as ready for review March 4, 2023 10:06
@circus2271 circus2271 force-pushed the feature/Show-other-markers-on-the-map-in-the-edit-mode branch from 33c10df to a245233 Compare March 4, 2023 10:08
@circus2271 circus2271 marked this pull request as draft March 4, 2023 10:51
@circus2271 circus2271 marked this pull request as ready for review March 4, 2023 11:07
Copy link
Member

@kabalin kabalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work @circus2271, see the comments.

this.pointHighlightDestroy().pointEditCreate().markerManager.disable();
let highlightedPhotoLayer;

this.markerManager.layerPhotos.eachLayer(function (marker) {
Copy link
Member

@kabalin kabalin Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest instead of looping over markers and disabling event, use this.editing flag (in map controller) in the popupPhotoOver and clickMarker event processing functions in the marker.js (in those functions do not do anything if editing mode is on).

});

if (highlightedPhotoLayer) {
this.markerManager.layerPhotos.removeLayer(highlightedPhotoLayer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not quite work, if you move editing pin out of screen by dragging the map and then drag it back, the point will be re-added, also this is not ideal to loop over all points every time we editing to find the right point (as pointed earlier). I suggest to refactor this in a way that cid would be used to identify the point. To do this:

  1. Keep cid of editing point in map controller object.
  2. When editing mode is on, identify current point using photos[cid].marker (markers module) and remove it from layer group (this.markerManager.layerPhotos.removeLayer).
  3. When new markers are added as result of panning/zooming map and editing is on, check if newly added point is matching editing cid, if so, do not add it to the layer group.
  4. When editing mode is off, add a new point marker only to the layer group, no need to refresh all.

Copy link
Contributor Author

@circus2271 circus2271 Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll see

thanks for your feedback

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(пока что отдыхаю)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @circus2271, are you planning to complete this at any time soon? If not I can take it over and finish (if you don't mind).

Copy link
Contributor Author

@circus2271 circus2271 Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't mind at all
That would be great, thanks

(I was too lazy to complete it)

@PastVu PastVu deleted a comment from circus2271 Mar 13, 2023
@circus2271 circus2271 marked this pull request as draft March 23, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants