Skip to content

Conversation

@mr21
Copy link
Contributor

@mr21 mr21 commented Apr 16, 2015

More info at the issue: #2219.

@mr21 mr21 force-pushed the jquery.easing._default branch 2 times, most recently from 43c50f9 to d2a077f Compare April 17, 2015 03:08
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer a default easing to stay at the Tween level, rather than here in Animate. And since @gnarf is back in this headspace for a bit, we can get a second opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote a resume of the situation in the other message.
But I like the way we put the default values and after the user rewrite them with the .extend.
I think (IMHO) we should do the same thing for the duration value.
And also for the complete callback with jQuery.noop maybe (it's only a suggestion).

@gibson042
Copy link
Member

I like the direction here, but please create an issue so we can track it. Also, the CLA failure will need to be fixed by amending this commit—and all future ones—to use your full name (which we require and are now verifying).

@mr21
Copy link
Contributor Author

mr21 commented Apr 17, 2015

hi @gibson042 :)
The issue is done.
In my .gitconfig I have exactly:

[user]
    email = thomastortorini@gmail.com
    name = Mr21
[push]
    default = simple

I signed the CLA again yesterday with my nickname and my email. I don't understand what I have to do exactly. Do I have to write my real full name?

@mgol
Copy link
Member

mgol commented Apr 17, 2015

Do I have to write my real full name?

Yes.

@mr21
Copy link
Contributor Author

mr21 commented Apr 17, 2015

Thanks @mzgol :)

I didn't respect exactly the principle of the atomic commit, so I redid my commit.
Now, only 3 lines change.

Actually, If we don't precise the easing the anim.originalOptions.easing will be undefined BUT
the anim.opts.easing will be the default value: "swing".

The duration don't respect the same idea. If I don't specify a duration the
anim.opts.duration and anim.originalOptions.duration will be equals to jQuery.fx.speeds._default (400). And I don't understand why there is also anim.duration directly.

@mr21 mr21 force-pushed the jquery.easing._default branch from d2a077f to 8732b95 Compare April 17, 2015 16:29
@mr21 mr21 force-pushed the jquery.easing._default branch from 8732b95 to df07ff7 Compare April 17, 2015 16:38
@mr21 mr21 mentioned this pull request Apr 17, 2015
@mr21
Copy link
Contributor Author

mr21 commented Apr 25, 2015

Hi,

I read in your meeting nobody share my code design 😢 so I moved the this.easing = easing || jQuery.easing._default; in the Tween.js file.

But should I move the code of the duration || _default in the Tween file also?

@mr21
Copy link
Contributor Author

mr21 commented May 3, 2015

I add two new tests for that but I have to change the variable's name of another test because it was using (and delete) the new jQuery.easing._default.

After you say it's better to see that in the Tween file I changed, but I didn't see the test animate with per-property easing (of the test/unit/effects.js file) failed.
It's concerne that:

jQuery(data).animate(
    {
        a: [ 100, "_test1" ],
        b: [ 100, "_test2" ],
        c: 100
    },
    400,
    "default_easing"
);

The property c should take the "default_easing" but this not work anymore with my 2nd commit.
But my first commit (when I change the effects.js file) works fine with this.

@gibson042
Copy link
Member

I'm having a hard time following this, so let me back up a bit and present a call stack as it appears in master:

Call State Notes
$elements.animate( { left: 0 }, null ) prop = { left: 0 }
speed = null
easing = undefined
callback = undefined
optall = jQuery.speed( speed, easing, callback ) optall = { duration: 400, easing: undefined, queue: "fx", old: false, complete: function(){ … } }
… (doAnimation) this = element0
Animation( this, jQuery.extend( {}, prop ), optall ) elem = element0
properties = { left: 0 }
options = optall
options passed to Animation are not original, but have already been created/modified by jQuery.speed
► ► animation = deferred.promise({ … }) animation = { originalProperties: properties, originalOptions: optall, duration: 400, props: { left: 0 }, opts: { specialEasing: {}, duration: 400, easing: undefined, … }, … }
props = animation.props
…so likewise for animation.originalOptions. animation.opts, however, is a copy that can be modified independently (e.g., by defaultPrefilter or by this proposal)
► ► propFilter( props, animation.opts.specialEasing )
► ► animationPrefilters[ index ].call( animation, elem, props, animation.opts )
► ► ► … (defaultPrefilter) this = animation
elem = element0
opts = animation.opts
► ► jQuery.map( props, createTween, animation )
► ► ► … (createTween) value = 0
prop = "left"
► ► ► ► collection[ index ].call( animation, prop, value )
► ► ► ► ► … (tweeners[ "*" ]) this = animation
► ► ► ► ► ► tween = this.createTween( prop, value ) end = 0
► ► ► ► ► ► ► `tween = jQuery.Tween( elem, animation.opts, prop, end, animation.opts.specialEasing[ prop ] animation.opts.easing )`
► ► ► ► ► ► ► ► … (jQuery.Tween.prototype.init) tween.elem = element0
tween.prop = "left"
tween.easing = "swing"
tween.options = animation.opts
tween.start = this.now = 100
tween.end = 0
tween.unit = "px"
tweens require defined easing, so initialization applies a default if there's no explicit (e.g., animation-level) setting
► ► jQuery.fx.timer(…)
► ► ► jQuery.fx.start()

Having gone through all of that, I think I understand what you're after: a guarantee of animation-level opts.easing. There's a potential problem from specialEasing (since properties are allowed to ease independently), but that is admittedly a rare case, and would still tolerate the animation-level default anyway. Note, however, that http://jsfiddle.net/Mr21/wh5sdgzu/ will not tolerate such variance because it looks for easing in anim.opts instead of a per-prop anim.opts.specialEasing[<prop>] or anim.tweens[<N>].

So the bottom line here is that I would be fine with use of jQuery.easing._default in both Animation and Tween.prototype.init. Tests for the former would assert that $collection.animate( props ) creates an animation with anim.opts.easing matching a custom jQuery.easing._default, and tests for the latter would do the same against anim.tweens[<N>].easing (and not matching for specialEasing tweens). Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Let's name this to better match its purpose, something like jQuery.easing.animationScope (and the other one to e.g. jQuery.easing.propertyScope).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah right, it's done.

@mr21
Copy link
Contributor Author

mr21 commented May 4, 2015

@gibson042 nice summary!

So the bottom line here is that I would be fine with use of jQuery.easing._default in both Animation and Tween.prototype.init.

I didn't understand exactly where do you think I have to put the || jQuery.easing._default.
Because if I put it in the opts : $.extend( ... ) (like my first commit) I don't have to repeat it in the Tween.init part. I'm confused 😕

@gibson042
Copy link
Member

jQuery.easing._default will be referenced twice. You need it in Tween.prototype.init (as in f4a3c787) because tweens can be created outside of Animation, and you want it in Animation (as in df07ff70) so animation objects always have an "easing" property.

@mr21
Copy link
Contributor Author

mr21 commented May 4, 2015

because tweens can be created outside of Animation

Ahhh okay, I didn't know this (I push another commit).

In Qunit I read (on my test):

3. No timers are still running
Expected: 0
Result: 1

Where is this test? And what I have to do to passe it?

@gibson042
Copy link
Member

That looks like one of our beforeEach/afterEach assertions, and indicative of an async issue (e.g., jQuery.fx.stop won't be called until the next tick).

@mr21
Copy link
Contributor Author

mr21 commented May 4, 2015

Okay, I add this.clock.tick( 25 ); and it's work fine.

timmywil added a commit that referenced this pull request May 5, 2015
@timmywil timmywil closed this in 5f2ea40 May 5, 2015
timmywil added a commit that referenced this pull request Nov 10, 2015
@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.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants