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

Ticket 7067 - Bounce and Pulsate durations are not accurate #168

Merged
merged 9 commits into from May 10, 2011

Conversation

@gnarf
Copy link
Member

commented Mar 27, 2011

They also didn't respect the "queue" order - if other animations were queued after the effect, they would run first.

http://bugs.jqueryui.com/ticket/7067

gnarf added 2 commits Mar 27, 2011
…ke 1000 ms - Fixes #7067 - Also making sure the queued animations run DIRECTLY after the effect
… instead of callbacks can help us see if we have places where we destroyed the queue
});
$(this).addClass("current")
// delaying the initial animation makes sure that the queue stays in tact
.delay( 10 )

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Apr 4, 2011

Member

Why is this delay necessary? Shouldn't the queue stay in tact regardless? If there's different behavior when there's already a queue of animations, then that seems like a bug.

This comment has been minimized.

Copy link
@gnarf

gnarf Apr 4, 2011

Author Member

Alright, if the delay isn't there, the hide animation runs immediately, therefore queuing its animations in the correct location, then the second delay happens, and then the show happens, also queueing animations correctly. So - The effects SEEM to happen correctly...

Before I fixed the bug in bounce/pulsate with moving the queue back in place... IF we add the delay this is what happens:

queue delay, queue hide, queue delay, queue show, execute hide which QUEUES (at the end) the effect, execute DELAY, execute SHOW which queues more animations (at the end), execute animations....

I added the delay in the test to ensure that we weren't HIDING the fact that the queue is being executed in the wrong order if you use the queue from within the effect...

times = o.times || 5,

// number of internal animations
anims = times * 2 + showhide,

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Apr 4, 2011

Member

I don't think we should be using booleans for addition. Clear code is more important than concise code.


// number of internal animations
anims = times * 2 + showhide,
speed = (o.duration || 250) / anims,

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Apr 4, 2011

Member

Since we're fixing the duration, we should fix this random default. All animations should default to $.fx.speeds._default.

This comment has been minimized.

Copy link
@gnarf

gnarf Apr 4, 2011

Author Member

Agreed... Will update

});
}

// inject all the animations we just queued to be first in line (after "inprogress")

This comment has been minimized.

Copy link
@scottgonzalez

scottgonzalez Apr 4, 2011

Member

Why do we do this?

This comment has been minimized.

Copy link
@gnarf

gnarf Apr 4, 2011

Author Member

Because we ALWAYS want the animations we just queued to execute immediately after the effect executes, even if there are more functions already on the queue stack

@gnarf

This comment has been minimized.

Copy link
Member Author

commented May 1, 2011

Tossed a few more cleanups at this pull request, got rid of some magical boolean math.

I tried a few approaches to queueing these animations in the correct location, but nothing seemed to want to work as cleanly as the queue splicing... We can't use any queue other than fx for .animate() it seems... I might have another solution available, but it doesn't look very pretty without adding a method to the $.fn (which might work well if we went with my jQuery.sub() patch)

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented May 3, 2011

The distance of the bounces changed with this commit. The element used to bounce higher.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented May 3, 2011

The first pulse in the pulsate effect is too fast.

@gnarf

This comment has been minimized.

Copy link
Member Author

commented May 3, 2011

Fixed the pulsate effect by switching the default mode to "effect" (its only not specified when you use the .effect() method...)

Bounce distances got normalized because they used to be different for hide vs show, that is the reason for that change...

@gnarf gnarf reopened this May 3, 2011
@scottgonzalez scottgonzalez merged commit 126d46c into jquery:master May 10, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.