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

Resolve #556: Improve geometry editing UI #720

Merged
merged 1 commit into from
Sep 26, 2016
Merged

Conversation

seav
Copy link
Contributor

@seav seav commented Sep 23, 2016

Proposed changes in this pull request

  • Resolve Editing location geometry is not intuitive #556:
    • Overview: Editing project boundaries and location geometries will be made simpler because the user does not have to click as much:
      • When the geometry edit page loads, the Leaflet.draw editing mode will be enabled automatically so that the geometry is immediately editable. This will be done by dispatching the "click" event on the "Edit" button on the map toolbar.
      • When the user clicks the big "Save" button on the page, any changes to the geometry will be automatically saved as well. This will be done by dispatching the "click" event on the small "Save" button on the map toolbar if it is still visible.
    • Add enableMapEditMode and saveOnMapEditMode functions to core/static/js/map_utils.js. These take care of two behaviors described above respectively.
    • Add JavaScript code/event listeners to call the two functions above to the project_edit_geometry.html and location_edit.html templates.
  • Partially resolve Canceling adding/editing a location does not work #719:
    • Fix the non-working cancel button during location editing by providing the location detail page as the cancel button's URL instead of the HTTP referrer.
    • Note: the cancel button during location creation is still not fixed as this requires a decision on the best cancel URL: it can be the referrer, but this must be preserved somehow, or just use the project dashboard URL.
  • Other improvements:
    • Make the indentation of core/static/js/map_utils.js consistent to 2 spaces.

When should this PR be merged

Anytime.

Risks

No risks foreseen.

Follow up actions

Since a static JavaScript file was modified, people need to clear their browser cache.

@seav
Copy link
Contributor Author

seav commented Sep 23, 2016

Note: I am not 100% satisfied with this PR because I prefer to hide the edit/delete buttons from the map toolbar when the existing geometry is being edited. But this makes things a bit messier for a couple of reasons:

  1. These buttons are the only way to access the editing mode and the save function because Leaflet.draw does not expose the low-level functions (like any good JavaScript library), so hiding them while being able to programmatically click on them is not reliable.
  2. If the user decides to replace the geometry (such as converting a point location to a polygon location), these buttons need to be shown so that that user can edit the new geometry right after drawing them. Initially hiding and then showing these buttons reliably can become messy.

@seav seav force-pushed the enhancement/#556 branch 3 times, most recently from 874eb23 to de221df Compare September 26, 2016 12:24
Copy link
Member

@oliverroick oliverroick left a comment

Choose a reason for hiding this comment

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

I agree on your comments about hiding the edit button. I'm not sure how to proceed here. A part of me says, it works and it makes the workflow much simpler but then I think this could be better. I think, we have already reached the limits of what we can do with leaflet-draw. It's a very simple library; leaflet-editable for instance is much more flexible and allows us to make the editing process more intuitive, but it probably requires some work to integrate it with django-leaflet. Maybe we should just stick with it for the time being. @wonderchook @ian-ross Any opinions about this>

function enableMapEditMode() {
var editButton = $('.leaflet-draw-edit-edit')[0];
if (!editButton) {
setTimeout(enableMapEditMode, 500);
Copy link
Member

Choose a reason for hiding this comment

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

I generally don't like timeouts to check whether elements are present on the page, but I assume Leaflet-draw does not fire an event when it's ready?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree that that's bad. The only legitimate use for setTimeout in JS code (unless you're using it to schedule tasks) is with a timeout of zero to defer processing until next time round the event loop. Putting actual timeouts in there is a recipe for disaster and very unpredictable bugs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'm probably not the right person to ask about this. I find Leaflet.draw to be pretty nice, myself, and I don't find the workflow at all unintuitive. But I'm not the target audience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, there's no event to indicate that the Leaflet.draw toolbar is ready. There's a core Leaflet event to say that the map itself is ready but none for the draw toolbar. I am very, very open to suggestions on a better way to check whether the draw toolbar is present.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an element you can check for? If so, you can poll for it every 100 ms or so until it's there. (That's a legitimate use of setTimeout!) But a one-shot timer that may or may not leave enough time for things to get set up isn't a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ian-ross, that is actually what I'm doing. I'm polling the presence of the edit button. enableMapEditMode calls itself with a setTimeout if the element is still not present.

Copy link
Contributor

Choose a reason for hiding this comment

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

D'oh. I didn't notice that. In that case, I think this use of setTimeout is fine! Very good! Sorry for the confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think at some point we are going to have to prioritize making the mapping much more friendly. (And add some requested features to make the geometry drawing more accurate). So I think this is fine for the time being. If any other requests come through related to the drawing we'll need to think through making it a larger feature to improve things.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds reasonable, I'm going to merge this.

@oliverroick oliverroick merged commit c67ea89 into master Sep 26, 2016
@oliverroick oliverroick deleted the enhancement/#556 branch September 26, 2016 15:39
manoramahp pushed a commit to manoramahp/cadasta-platform that referenced this pull request Sep 27, 2016
manoramahp added a commit to manoramahp/cadasta-platform that referenced this pull request Sep 27, 2016
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.

Canceling adding/editing a location does not work Editing location geometry is not intuitive
4 participants