-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat(pauseResume): pause/resume feature(jQuery plugin) #151
Conversation
Merge remote-tracking branch 'naver-egjs/master'
}; | ||
|
||
$.fn.pause = function() { | ||
// console.warn("===========PAUSE=============="); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove console comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I'm now cleaning the code and adding useful comments at the code.
I have reviewed. |
}; | ||
} | ||
|
||
function now() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'd better use jQuery.now()
http://api.jquery.com/jquery.now/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sculove Wow! Good!
I propose |
|
||
if (!prevProp[propName]) { | ||
// If this makes performance issues, then change the way of calucating relative value. | ||
computed = computed || getStyles(el); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
computed
is always undefined
. Why does it check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If prop
has a more than one property, then computed
can be used.
For example, prop has below.
prop = {
left: "+=100px",
width: "+=100px",
height: "+=100px"
}
Without computed, below code can call getStyles 3 times in above case.
var computed;
for (var propName in prop) {
//...
if (!prevProp[propName]) {
computed = computed || getStyles(el);
//...
}
//...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah.. Sorry. I didn`t find for loop. :)
LGTM |
var uuid = 1; | ||
var getStyles; | ||
|
||
if (window.getComputedStyle) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why didn't you use jQuery
method? I think that It didn't working including dash style.(like background-color
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mixed
I considered to use jQuery method and I found that getStyle
function is implemented as a local function. So I looked for external function that wraps the getStyle. But I couldn't find.
I found 2 use cases of getStyle
on external function.
- $.cssHooks["height" | "width"].set = function() {}
- $.fn.css
First one is not correspond with purpose,
Second one needs property name to get specific value.
Can you help me to find the substitution for the above code? :)
And I can't understand the situation where dash style is included. Do you mean that getComputedStyle
can return dash-style value? or element can have dash-style value (So it cannot be obtained through getComputedStyle
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- When use animate in IE8(link) that Including dash style(like
padding-left
) didn`t working. - It have performance issue using
getComputedStyle
. But If You don't change DOM It haven't issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mixed Thank you for your kindness. And I applied your reviews. As you tell me, I tested that While a DOM element is not changing, the getComputedStyle
makes tiny performance issue which we don't have to worry. I think Changing from caching the getComputed value as a computed
variable to $.css will be OK.
But I want to emphasize other BIG differences on this commit.
I changed the time to calculate absolute value from relative value and the way to get previous value.
BEFORE
- calculation time: on animate call time
- way to get previous value: cached value each calculated on animate call time.
AFTER
- calculation time: on animate complete time
- way to get previous value: call $.css when current value is relative.
Problems that 'BEFORE' had is below.
CSS value can be changed between before animation is done like below.
this.$el
.animate({"left": "100px"}, duration, function() {
/**
* This makes an changes.
*/
$.style(this, "left", 0);
})
.animate({"left": "+=100px"});//MUST move left value based on 0.
Code above does not act on BEFORE pause/resume code. because Next animation refers to the value that is not applied custom changes(like on complete, step, progress)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jongmoon
I got it. LGTM!
Issue
#111
Details
jQuery Animate - pause/resume plugin feature
Preferred reviewers
@naver/egjs-dev