Skip to content

Conversation

@gibson042
Copy link
Member

9d822bc failed an effects test (cf. #3470 (comment) ), but it turns out that things were already broken in subtle ways when animations spawn each other:

  • immediately-done animations miss the final progress notification
  • callbacks are registered after the first tick
  • animations get skipped when old ones are forcefully stopped mid-tick

Each fix revealed the next flaw, so all are addressed here in order to resolve the current CI failure. I should probably make real issues so they can be resolved by this PR.

@mention-bot
Copy link

@gibson042, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gnarf, @markelog and @dmethvin to be potential reviewers.

src/effects.js Outdated
animation.opts.start.call( elem, animation );
}

// attach callbacks from options
Copy link
Member

Choose a reason for hiding this comment

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

Stupid nitpick: probably better to start with uppercase 'attach' -> 'Attach', I know it was like that before and linter didn't catch that for some reason...

Copy link
Member

@markelog markelog left a comment

Choose a reason for hiding this comment

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

Looks like this commit head a little long, no?

@markelog
Copy link
Member

This is awesome, nicely done, well, as always! :)

@timmywil
Copy link
Member

Any chance this affects #3434?

@gibson042
Copy link
Member Author

I wouldn't think so, but it's possible. #3434 is unexpected.

@gibson042 gibson042 merged commit 3c89329 into jquery:master Jan 16, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants