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

Correctly finish scroll animation when using page keys #1244

Merged
merged 1 commit into from Jun 21, 2017

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Jun 20, 2017

Fixes #1022.

When .finish() is called on an element, the currently-running animation and all queued animations (if any) immediately stop and their CSS properties set to their target values. All queued animations are removed.

@xPaw xPaw added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Jun 20, 2017
@xPaw xPaw added this to the 2.3.2 milestone Jun 20, 2017
@@ -1306,15 +1306,14 @@ $(function() {
"pagedown"
], function(e, key) {
let container = windows.find(".window.active");
if (container.is(":animated")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This if should have been lower by the way, so it doesn't really work anyway.

@AlMcKinlay AlMcKinlay self-requested a review June 20, 2017 10:28
Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

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

Haven't tested but looks fine to me.
@YaManicKill, let me know if you want to test it or not and if not I'll do that later, so at least one of us tests it.

@AlMcKinlay
Copy link
Member

@astorije I'm not bothered, I just want one of us to test it. If you get to it first, feel free. I'll let you know if I manage to get some time to do it this evening or not.

Copy link
Member

@AlMcKinlay AlMcKinlay left a comment

Choose a reason for hiding this comment

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

So, it does owrk, but it goes incredibly quickly. It's probably better than what we have now, but definitely not ideal.

@astorije
Copy link
Member

Anything you think can be improved, @xPaw?

@xPaw
Copy link
Member Author

xPaw commented Jun 20, 2017

Initially I wanted to get jQuery's animation state, so I could get final scrollTop target, stop the animation and start a new one with adjusted scrollTop, but there jQuery provides no way of doing that. I could be storing scroll data myself to re-use, but I don't know if it's worth it?

@astorije
Copy link
Member

I could be storing scroll data myself to re-use, but I don't know if it's worth it?

It's probably worth a try, but if this PR already improves things, that's a win IMO. I'm going to merge this as-is, and feel free to attempt something better (but probably more experimental) for a future improvement.

Either way, thanks for that!

@astorije astorije merged commit d09ac7a into master Jun 21, 2017
@astorije astorije deleted the xPaw/fix-page-keys branch June 21, 2017 03:25
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Correctly finish scroll animation when using page keys
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants