-
Notifications
You must be signed in to change notification settings - Fork 31
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
Use Leaflet.Draw fork that supports touch #502
Conversation
@azavea-bot rebuild |
Thanks for listing out the relevant considerations to evaluate. As we discussed offline, though this should make us a little nervous, the touch support is something of must-have for the client, so this is a necessary risk. I think we just need to remain active on the issue/PR, encourage its getting merged and leave an outstanding issue in our repo to follow up when it is. If instability or other problems occur, we'll have to re-evaluate then, but I'm at least buoyed by the community response to this fork. |
Nice write up. |
I've just updated the main comment with more testing information. The general feel is:
I will test further today. |
Created #504 for Internet Explorer issues. |
A potential large deal breaker is that click functionality is impeded in Chrome on Windows when running on a touchscreen: The polygon refuses to close, and clicking on the starting point simply adds more overlapping points instead of closing the polygon. Internet Explorer won't even render the page. This can be alleviated by adding a button, only visible during drawing, that allows one to close the polygon. |
I got confirmation from Stroud today that they are comfortable supporting iPad and desktop browsers specifically, even knowing the current limitations with the dual-input devices like the Surface. We should track the life of this fork so we can revert to the origin, but let's deal with "minor" devices and their problems as their reported. |
I'm subscribed to the original issue and the pull request, as well as the fork, so should be notified of any activity there. I'm going to rebase this on top of |
Created #526 and #527 for Android and iPad issues respectively. While this PR adds touch support, we need some more work to polish the UX in touch devices. Those issues are there to track whatever issues we discover as we test. Rebasing on |
Use Leaflet.Draw fork that supports touch Connects #476
Hey guys, I'm working on a leaflet.draw fork that will be needing IE support for drawing polygons etc. Just wanted to inject myself into this conversation going forward. Cool work with watershed by the way! |
I just got a report that this problem is also coming up in touchscreen Chromebooks. |
Introduction
We are switching to Michael Guild's fork of Leaflet.Draw to get support for touch screens. There are a number of considerations that must be taken into account.
Original Issue
The original issue on Leaflet.Draw is here: Leaflet/Leaflet.draw#2. The conversation on that thread started with a few contributors who came up with various solutions which worked in some cases and didn't in others, until Guild came along and essentially took ownership of the effort, and has been working on it on-and-off for more than a year now. As the conversation moves along, the comments get more and more supportive and appreciative, and the bugs become rarer and more edge-case-y. There is also some discussion about Tom Blackmore's fork, and Jacob Toye (who seems to be a core developer of Leaflet.Draw) initially seemed to favor his approach. However, Guild's fork has been more active, has been used and tested by more people, and is up to date with the current Leaflet.Draw codebase.
Pull Request
Guild has an open pull request to the main repo here: Leaflet/Leaflet.draw#285. Initial conversation is about syntax, but eventually people start testing it and liking it, and Toye chimes in with his approval and a promise to carve out time for Leaflet.Draw in the future. The main issues people have with the PR are:
But overall consensus is positive, with the hopes that his work will be merged into the main Leaflet.Draw repo at some point in the near future. The main blocker for that effort is simply time on the part of both Guild and Toye, who had not been able to pay much attention to this, until recently when activity has started again.
Concerns
master
, thus we are not just getting the touch support, but all sorts of changes to Leaflet.Draw.package.json
. All our existing tests are passing, and prelimnary testing in Chrome and Mobile Safari on iOS has been flawless. But we need to test more.Testing
To test this, check out the PR branch and run
Then check with various browsers and devices that the map is usable:
Tested on:
/analyze
view. Modifications needed to be tapped twice, the first time simulated a hover and the second a click./analyze
view.mouseclick
event. Each touch acts as a hover /mousemove
event instead of a click.❌ = Major problems related to touch functionality
I will leave this PR open until I've tested at least on the above platforms.
Connects #476