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

Already on GitHub? Sign in to your account

Changing order of scroll properties alteration so changing the scroll target won't mess with the 'elem' reference wrongly. Adding minified version and grunt config file. #24

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
2 participants

I had a problem where overthrow would not scroll to the edges of a container, because it'd figure there was no room anymore for scrolling, and would then change the event target to enable nesting before actually finishing the original scroll event. Changing one if's position solved the problem with no undesired consequences. I also added a config file for grunt, and a lint'd and minified version of overthrow.

I found 2 more issues recently, and managed to fix them too.

  • The touchend event dispatched manually on changeScrollTarget had no touches, and any other lib that manipulates touch events would get an undefined on that. To avoid that I copied the touches from the touchmove event that caused the change.
  • When scrolling to one of the extremes of the overthrow-marked container, there would always be a change of the scroll target, even if there was no possible target up on the DOM tree. This caused the scroll gesture to end prematurely when the user still wanted to keep scrolling that container. I fixed this by checking if there is a candidate up on the tree to be the next target, otherwise I'll preventDefault() and maintain the target so scrolling can continue.

I also cherry-picked two of the currently open pull-requests (from @AshKyd and @cburgdorf), did a clean up on the tests to follow @cburgdorf 's changes, and updated the minified version.

I can confirm it's working on androids ranging from 2.2 up to 3.2 and the tests are passing for me.

Owner

scottjehl commented Jul 24, 2013

Unfortunately, the code has diverged a bit from when these changes were made, and some features that are fixed here are no longer in the approach at all. I'm hoping to circle back and look at some of the things you did here when we get a chance, and see if any should be merged in. But this PR in particular can't be merged so I'm going to close it out. Thanks again.

@scottjehl scottjehl closed this Jul 24, 2013

When I updated the PR 8 days ago this master branch was not a single commit ahead of mine, sorry for that. However, I went through my changes and the new commits made since when I cloned and I still think the code changes applies, maybe even without conflicts. I didn't even touch the 'toss' mechanism.

Yet I understand your reasoning. I'll revert my branch to the same whitespace config that you seem to use to make it easier, if you ever want to merge this back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment