Skip to content

Effects: finish should call progress #2292

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.

Sign up for GitHub

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

Jump to bottom
Closed
mr21 wants to merge 11 commits into from
Closed

Effects: finish should call progress #2292

mr21 wants to merge 11 commits into from

Conversation

@mr21
Copy link
Contributor

@mr21 mr21 commented May 11, 2015

A pull request for #2283.

@dmethvin
Copy link
Member

dmethvin commented May 11, 2015

Thanks! We will need some unit tests for this change to ensure that it actually works. The closest existing test we have is here but you can probably create a simpler one just to check for .finish() calling the callback and the .deferred().

gibson042
gibson042 reviewed May 11, 2015
View reviewed changes
src/effects.js Outdated
Copy link
Member

@gibson042 gibson042 May 11, 2015

Choose a reason for hiding this comment

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

Missing space between ] and ).

Copy link
Contributor Author

@mr21 mr21 May 11, 2015

Choose a reason for hiding this comment

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

Oops, it's okay.

@gibson042
Copy link
Member

gibson042 commented May 11, 2015

I was on the fence about changing when we notify animations, but this makes sense. 👍

@timmywil
Copy link
Member

timmywil commented May 11, 2015

Agreed 👍 Thanks @mr21!

mr21
mr21 reviewed May 11, 2015
View reviewed changes
test/unit/effects.js
Copy link
Contributor Author

@mr21 mr21 May 11, 2015

Choose a reason for hiding this comment

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

I change something here, because in jQuery if there is no property to animate (here it's empty with the {}) .stop( true ) is called. So, progress is now called twice.

@mr21
Copy link
Contributor Author

mr21 commented May 11, 2015

👍
@dmethvin, yep I wrote a new test and modify another one.

gibson042
gibson042 reviewed May 11, 2015
View reviewed changes
test/unit/effects.js Outdated
Copy link
Member

@gibson042 gibson042 May 11, 2015

Choose a reason for hiding this comment

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

Just prog = 1, please. There's no need to be overly clever in known environments like unit tests.

Copy link
Contributor Author

@mr21 mr21 May 11, 2015

Choose a reason for hiding this comment

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

Yeah right (done).

arthurvr
arthurvr reviewed May 11, 2015
View reviewed changes
test/unit/effects.js Outdated
Copy link
Member

@arthurvr arthurvr May 11, 2015

Choose a reason for hiding this comment

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

why aren't we using a camel cased variable name here, as always?

Copy link
Contributor Author

@mr21 mr21 May 11, 2015

Choose a reason for hiding this comment

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

It's because I'm used to add jq_ for every jQuery object, but I removed the variable anyway, thanks.

@mr21
Copy link
Contributor Author

mr21 commented May 12, 2015

@gibson042, I've used qunit-assert-step (like you said) to test if the callbacks are called in order. It works fine :)
I put the plugin in the external directory with qunit, is it the good way?

@gibson042
Copy link
Member

gibson042 commented May 12, 2015

Yep; looks like the plugin was added correctly. 👍

gibson042
gibson042 reviewed May 12, 2015
View reviewed changes
test/unit/effects.js Outdated
Copy link
Member

@gibson042 gibson042 May 12, 2015

Choose a reason for hiding this comment

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

This assertion adds no value.

Copy link
Contributor Author

@mr21 mr21 May 12, 2015

Choose a reason for hiding this comment

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

Yeah right, I removed this

@mgol
Copy link
Member

mgol commented May 12, 2015

@mr21

@gibson042, I've used qunit-assert-step (like you said) to test if the callbacks are called in order. It's works fine :)
I put the plugin in the external directory with qunit, is it the good way?

Yep; looks like the plugin was added correctly. 👍

Actually, it wasn't. ;) You shouldn't copy files to external/ by yourself, you should add proper files to the npmcopy config and then invoke grunt npmcopy which will do the copying. Then you need to commit those copied files.

gibson042
gibson042 reviewed May 12, 2015
View reviewed changes
test/unit/effects.js Outdated
Copy link
Member

@gibson042 gibson042 May 12, 2015

Choose a reason for hiding this comment

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

These equals should be strictEquals, and the messages ought to be more clear (e.g., "(first|last) progress callback: (progress ratio|remaining ms)").

Copy link
Contributor Author

@mr21 mr21 May 12, 2015

Choose a reason for hiding this comment

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

I renamed these 4 tests, thanks

@mr21
Copy link
Contributor Author

mr21 commented May 12, 2015

@mzgol, okay thanks, I modified the Gruntfile.js, it works.

@mr21
Copy link
Contributor Author

mr21 commented Jun 11, 2015

No more conflict here

@mgol mgol closed this in 3dd3d13 Sep 8, 2015
mgol pushed a commit that referenced this pull request Sep 8, 2015
@mr21 @mgol
Effects: Finish should call progress
f71e32d 
(cherry-picked from 3dd3d13)

Fixes gh-2283
Closes gh-2292
@mgol
Copy link
Member

mgol commented Sep 8, 2015

Landed, thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mr21 @dmethvin @gibson042 @timmywil @mgol @arthurvr @jquerybot