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

Polyline fixes #657

Closed
wants to merge 40 commits into from
Closed

Polyline fixes #657

wants to merge 40 commits into from

Conversation

z0d14c
Copy link

@z0d14c z0d14c commented Dec 14, 2016

After much adieu, I have a PR that I've tested on Chrome/iOS/Android/IE and appears to solve the following issues reliably:

  • IE clockwise issues
  • mobile touch issues (closing polylines/polygons)
  • a few other issues that i noticed as i was knocking down these issues, such as distance tooltip throwing errors on mobile views

description of code changes

  • enables a flag, uses a timeout to briefly disable duplicate polyline touchevents
  • binds touch/click events more discriminately based on the browser
  • disables distance tooltip which was throwing errors on touch devices
  • additional touchHandled/clickHandled flags for safety
  • misc cleanup, more intelligent logic in some places

these changes result in a much more cross-platform stable version of leaflet-draw, and so I hope that despite the lack of tests, it can be merged, as I may not get a chance to write tests anytime soon. i tried to tag in all of the issues i believe that it probably fixes.

.on('mousemove', this._onMouseMove, this) // Necessary to prevent 0.8 stutter
.on('mousedown', this._onMouseDown, this)
.on('mouseup', this._onMouseUp, this) // Necessary for 0.8 compatibility
Copy link
Author

Choose a reason for hiding this comment

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

these got shuffled around as i was experimenting with fixes, settled on no change here. i was trying to conditionally bind these listeners based on L.Browser.touch, but that is unreliable because, for example, windows touchscreen devices may need to support both types of events.

@ddproxy
Copy link
Member

ddproxy commented Dec 18, 2016

@z0d14c It's pretty close, I squashed it over in #661 - I have some notes for a few little quirks in that PR.

@bart
Copy link

bart commented Jan 2, 2017

Any progress with this?

@ddproxy
Copy link
Member

ddproxy commented Jan 3, 2017

This has been merged with #661

@ddproxy ddproxy closed this Jan 3, 2017
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.

3 participants