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

Fix drag event on the first pixel moved Fix #1928 #1972 #1934

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fix drag event on the first pixel moved Fix #1928 #1972 #1934

wants to merge 2 commits into from

Conversation

lateek35
Copy link

I created this pull request in order to resolve #1928 bug.

Currently the drag event is not fired for the first pixel moved, it only set the listener for the next pixel dragged.
It leads to a bug when the user drag for only 1 pixel (it unfortunately happened sometimes when you click fast in the UI) because this._drag.stage.current is not updated.
So when the onDragEnd event is fired the position is set to -1 because it never enter the if statement of the closest method. This is because coordinate shouldn't be 0 but 1.

if (this.op(coordinate, '>', coordinates[this.minimum()])) {
	position = coordinate = this.minimum();
} else if (this.op(coordinate, '<', coordinates[this.maximum()])) {
	position = coordinate = this.maximum();
}

@lateek35 lateek35 changed the title Fix drag event on the first pixel moved Fix #1928 Fix drag event on the first pixel moved Fix #1928#1972 Jul 21, 2017
@lateek35 lateek35 changed the title Fix drag event on the first pixel moved Fix #1928#1972 Fix drag event on the first pixel moved Fix #1928 #1972 Jul 21, 2017
@laurens94
Copy link

I've got the same issue, however after modifying the files with your changes, the problem still persists... :(

@lateek35
Copy link
Author

lateek35 commented Jan 16, 2018

@laurens94
Did you apply changes of this specific commit 2c65340 ?
By the way, there is somes changes in the commit that are not mine in the (dist), it's only because the commit before mine wasn't done with the project compiled, just ignore it.

Can you try to directly use this file and tell if you still get problem: https://github.com/lateek35/OwlCarousel2/blob/2c653406be0e0b164011dfd02cf4beb4735a9e1e/dist/owl.carousel.js ?

If not there is another way to get rid of this bug by replacing :

if (this.op(coordinate, '>', coordinates[this.minimum()])) {

by

if (this.op(coordinate, '>=', coordinates[this.minimum()])) {

even it's more like a "hot fix" but it works

@laurens94
Copy link

laurens94 commented Jan 18, 2018

@lateek35 The changes in the commit you mentioned didn't solve the problem for me, though your 'hot fix' did! Thanks! 👍

@laurens94
Copy link

@lateek35 Although it works, I now receive the following error on triggering destroy.owl.carousel:

ReferenceError: settings is not defined
at Navigation.destroy (owl.carousel.js:2899)
at Owl.destroy (owl.carousel.js:1411)
at Owl. (owl.carousel.js:1681)

@lateek35
Copy link
Author

@laurens94
Did you apply the fix on the official version of the plugin ?
I made a fiddlejs here http://jsfiddle.net/ctg5s0kx/.
As you can see, I directly copy paste the official owlcarousel.js into the JS window, and replace the specified line of the hot fix. And as you can see, after 5s, I destroy the carousel, but no error show up, everything works and and the bug disappear.
It's probably come from something else, but no idea what.

@laurens94
Copy link

@lateek35 That did the trick! I applied the fix on the commit mentioned before, so that was the problem.

Thanks a lot! :)

@bobsoap
Copy link

bobsoap commented Mar 8, 2018

@lateek35 Thank you! Your hotfix solved half the problem for me, but the bug kept appearing when the stage was scrolled beyond 0. I was able to fix it fully by enhancing the operator in the second condition as well, right after yours:

} else if (this.op(coordinate, '<=', coordinates[this.maximum()])) {

So now it compares >= against the minimum (your fix) and <= against the maximum (my addition). This solved it on my end.

@pascalporedda
Copy link
Collaborator

Hey @lateek35

thank you for your PR. It's been a while, I know, but would you mind rebasing and maybe integrating the changes from @bobsoap and can you prove that this is working?

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.

Drag from left to right of 1px issue
4 participants