PR for #21 #31
Merged
PR for #21 #31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
PR to get fork up to date
PR to update branch
sasha240100
reviewed
Aug 7, 2018
src/between.js
Outdated
| @@ -15,9 +15,10 @@ const _requestAnimationFrame = ( // polyfill | |||
| || raf | |||
| ); | |||
|
|
|||
| let _prevTime = Date.now(), _time, _delta; | |||
| let _prevTime = Date.now(), _time, _delta, _paused; | |||
There was a problem hiding this comment.
Won't _paused stop all loops instead of just one? @hirako2000
There was a problem hiding this comment.
yes it was i thought one instance per tween... why would they share state?
I think, if that's the case, one instance per tween would be preferable, furthermore, should move away from Date.now() to some sort of Loop /timerinstance instance, for finer grained control. (thinks backward action and timelapse value on each tween)
|
Yes, we can replace Date.now() with delta a second argument - then it would
work
…On Wed, Aug 8, 2018 at 1:39 AM Hirako ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/between.js
<#31 (comment)>:
> @@ -15,9 +15,10 @@ const _requestAnimationFrame = ( // polyfill
|| raf
);
-let _prevTime = Date.now(), _time, _delta;
+let _prevTime = Date.now(), _time, _delta, _paused;
yes it would i thought one instance per tween... why would they share
state?
I think, if that's the case, one instance per tween would be preferable,
furthermore, should move away from Date.now()l to some sort of Loop /timerinstance
instance, for finer grained control.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#31 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHTX1a-2tBr7JP3QInd2Regua_iZ86zKks5uOhcmgaJpZM4Vx0Rk>
.
|
… use Symbol for the paused flag , and replace function with proper getter sasha240100#21
hirako2000
commented
Aug 8, 2018
| return this; | ||
| } | ||
|
|
||
| get isPaused() { |
There was a problem hiding this comment.
added getter instead of function
hirako2000
commented
Aug 8, 2018
| @@ -9,6 +9,7 @@ const _betweens = []; | |||
| const SYMBOL_TYPE = Symbol('type'); | |||
| const SYMBOL_START_TIME = Symbol('start_time'); | |||
| const SYMBOL_COMPLETED = Symbol('completed'); | |||
| const SYMBOL_PAUSED = Symbol('paused'); | |||
There was a problem hiding this comment.
using symbol now.
|
thanks for pointing that out. |
sasha240100
reviewed
Aug 8, 2018
| return this; | ||
| } | ||
|
|
||
| get isPaused() { |
types/between-tests.ts
Outdated
| @@ -22,7 +22,7 @@ between.on('complete', (value) => { value }); | |||
| // Test pause control | |||
| between.pause(); | |||
| between.start(); | |||
There was a problem hiding this comment.
@hirako2000 How about renaming .start() to .run() ?
There was a problem hiding this comment.
I called it start because the emitted event that was already present is called start. It's for consistency.
But up to you, I can rename if you think it's better.
|
We better name it “play” * as it will continue paused tween
…On Wed, Aug 8, 2018 at 5:03 PM Hirako ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In types/between-tests.ts
<#31 (comment)>:
> @@ -22,7 +22,7 @@ between.on('complete', (value) => { value });
// Test pause control
between.pause();
between.start();
I called it start because the emitted event that was already present is
called start. It's for consistency.
But up to you, I can rename if you think it's better.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#31 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHTX1cnErHxn5ZG32l44A1-XjLGvmmrWks5uOu-egaJpZM4Vx0Rk>
.
|
|
Renamed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds pause functionality and update first example to demo it