-
Notifications
You must be signed in to change notification settings - Fork 477
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
Animation API refactor #420
Conversation
- Made the methods on the prototype vs in constructor - Added back LayerStateMachine.reset()
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Object/keys
Given this, I think we could actually make |
So, this begs the question; what should this do? layer = new Layer
print "layer.stateNames", layer.stateNames
print "Object.keys", Object.keys(layer.states)
print "for..in", (k for k in layer.states) The current output is:
To me, it seems like:
|
All right, so we can't get I also have |
- We now always use initial, unless you are using deprecated methods. - .stateNames, Object.keys(layer.states) and for...of all work the same (except for for...in) - The initial state is managed by LayerStates again.
@koenbok about default/initial: have you seen: https://github.com/motif/company/issues/2624 |
Yeah :-/ let's chat about it tomorrow. |
Ok, re: https://github.com/motif/company/issues/2624 we decided to rip out the |
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.
Think all's good, have some minor suggestions and questions. If you have some general remarks about this refactor, I'd also like to hear them!
# Mix of current and old api | ||
if arguments.length is 2 | ||
layer = args[0] | ||
if args[1].properties |
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.
Shouldn't we check for null with
if args[1].properties?
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.
👌
properties = args[1].properties | ||
else | ||
properties = args[1] | ||
options = args[1].options if args[1].options |
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.
Same here args[i].options?
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.
👌
delete options.properties | ||
delete options.options | ||
|
||
# print "Animation", layer, properties, options |
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.
This can be removed
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.
👌
@@ -64,6 +96,9 @@ class exports.Animation extends BaseClass | |||
@_originalState = @_currentState() | |||
@_repeatCounter = @options.repeat | |||
|
|||
@define "layer", | |||
get: -> @_layer |
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.
I use later setting of layers in the animation preview of autocode. Are you ok with just using private api there?
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.
Yeah let's do that for now
@@ -195,6 +195,7 @@ class TouchEmulator extends BaseClass | |||
curve: "ease-out" | |||
|
|||
hideTouchCursor: -> | |||
return unless @touchPointLayer.opacity > 0 |
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 is this in this PR?
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.
:-/
#Support the old properties syntax | ||
if properties.properties? | ||
# console.warn "Using Layer.animate with 'properties' key is deprecated: please provide properties directly and use the 'options' key to provide animation options instead" | ||
# print "layer.animate", properties, options |
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.
Print comment can be removed
|
||
# Support the old properties syntax, we add all properties top level and | ||
# move the options into an options property. | ||
if properties.hasOwnProperty("properties") |
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 do we check hasOwnProperty
here instead of doing properties.properties?
# With the new api we treat the properties as animatable properties, and use | ||
# the special options keyword for animation options. | ||
if properties.hasOwnProperty("options") | ||
options = _.defaults(properties.options, options) |
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.
I think specifying options in the function should have precedence over specifying them in a options key
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.
Yep.
get: -> @_stateMachine.states | ||
get: -> | ||
@_states ?= new LayerStates(@) | ||
return @_states |
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.
I really dislike this behaviour of capturing the state inside of a getter, leads to unexpected behaviour. Can't we add it to the creation / addition of states?
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.
Well, I'd like that, but:
- There is no hook to see if a state gets created (because it's just an object key).
- This guarantees backwards compat
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.
Agree with 2), we could fix 1) if we ever move to initial, but am I correct that in order to drop this, we would also need to drop the backwards compatibility of default
?
for name, state of states | ||
@_stateMachine.states[name] = state | ||
@states.machine.reset() | ||
_.extend(@states, states) |
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.
We could capture the current state of the layer here, we should however be aware of the setting of states individually...
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.
Not really, because this should work too:
layer = new Layer
layer.states.test = {x: 500}
layer.animate "default"
|
||
it "should have an extra state", -> | ||
layer = new Layer | ||
layer.states.test = {x: 100} | ||
testStates(layer, ["initial", "test"]) | ||
testStates(layer, ["default", "test"]) |
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.
We could use initialStateName
here as well
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.
🏸
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.
Again, some minor remarks, but noting really blocking
return @animateToState(properties, options) | ||
# Support options as an object | ||
options = options.options if options.options? | ||
return @states.machine.switchTo(properties, options) |
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.
It would be nicer to put properties in a new variable stateName
here to make it more clear what is happening
states = [] | ||
states = _.flatten(args) if args.length | ||
@animate(@states.machine.next(states), options) | ||
|
||
stateSwitch: (stateName, options={}) -> | ||
@animate(stateName, _.defaults({}, options, {instant:true})) |
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.
Do we want to support animated: true
?
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.
Meh.
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.
Well ok.
@@ -21,7 +21,7 @@ class exports.LayerStateMachine extends BaseClass | |||
get: -> @_currentName | |||
|
|||
@define "previousName", | |||
get: -> _.last(@_previousNames) | |||
get: -> _.last(@_previousNames) or "default" |
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 do we need this? (I don't understand the case where _.last(@_previousNames) is null)
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.
I think it's better to always return something here so you don't have to do a null check (if you just created a layer and ask for layer.states.previous.x
.
@@ -15,6 +15,9 @@ deprecatedWarning = (name, suggestion) -> | |||
message += ", use '#{suggestion}' instead." if suggestion? | |||
console.warn message | |||
|
|||
namedState = (state, name) -> | |||
return _.extend({}, state, {name: name}) |
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.
Don't we want to enforce the named state by reversing state
and {name: name}
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.
Yes.
@@ -3,6 +3,9 @@ assert = require "assert" | |||
|
|||
initialStateName = "default" | |||
|
|||
cleanState = (state) -> | |||
return _.pickBy(state, (value, key) -> key isnt "name") |
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.
This could have a better name then cleanState
, maybe removeKeysFromState(state,keys=["name"])
?
@@ -158,24 +185,24 @@ describe "LayerStates", -> | |||
# it "should be a no-op to change to the current state", -> | |||
# layer = new Layer | |||
# layer.states.stateA = {x: 100} | |||
# layer.switchInstant "stateA" | |||
# layer.stateSwitch "stateA" | |||
# animation = layer.animate "stateA", time: 0.05 | |||
# assert.equal(animation, null) |
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 is this test disabled?
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.
Because the behaviour changed. We actually do always return an animation now so that the events get called and you can skip a null 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.
Great. Then it can be removed instead of commented out
Review @nvh? |
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.
LGTM
Ok here we go. |
Update: this now lives here: https://github.com/koenbok/Framer/wiki/New-Animation-API
Continuing from #378
API Overview
This is a current relevant overview of how the API will be from now on. For more information about what changed, see below.
layer.animate(properties: object | state: string, options={})
Returns: Animation object
animationOptions
).This creates a new animation for a layer based on a state or new property values and starts it immediately. If the properties have an
options
key, it is used as options for the animation.If there is currently an animation on the layer, it gets stopped if it is animating the same properties as the new animation. Simply put, no two animations can be ran on width at the same time, but you can have two separate animations, one running on width and the other on height.
Preferred Examples
Simple examples for animating of properties.
Simple examples for animating with states.
layer.animateStop()
Stop all animations for this layer.
layer.animationOptions
Animation objects manage animations that target a layer and properties. An animation will tween between a start and end value, with a curve. The start value is determined when the animation starts, and the end value is defined by properties. If the start values equal to end values, they won't animate.
curve
— A string, set to ease by default. (Optional)curveOptions
— An object with the options of the set curve. (Optional)time
— A number, the duration in seconds. (Optional)delay
— A number, the delay of the animation. (Optional)repeat
— A number, the amount of times it repeats. (Optional)colorModel
— A string, the model to animate colors in. (Optional)instant
— A boolean, don't animate. (Optional)layer.states
(object)The states object on a layer holds all the different states for a layer. They can be used to animate to, switch to, or cycled through. States are a great way to organize different visual properties of layers.
The properties of a state can be anything on a layer like
x
,y
, etc. Properties that cannot be animated likehtml
orvisible
can still be used but will not be animated. They will get set at the end of a transition.States can have a special
animationOptions
key to hold animation options likecurve
. They will be used whenever layers get animated to that state.There are two special states:
current
andprevious
. They refer to the current and last state that a layer is in. Additionally, the have aname
key so you can check the current state name withlayer.states.current.name
.Examples
Here is a common example where we set multiple states and cycle through them.
layer.statesCycle(states: array<string>?, options={})
Changes overview
Animation
Change a property.
Change a property with options.
Change the animation curve.
States
Add a single state.
Define multiple states at once.
Notice the subtle difference between calling a function and setting a property. This means that where previously it was possible to add multiple states multiple times, in the new API we will override the existing states when calling
layer.states = ...
again. However, this is really unlikely and one could still achieve this by doing:Animate to state.
Animate to state with options.
States can now also include animation options in the state itself. This is handy for declaring how to animate to a state as well. This has the exact same result as above.
Instantly switch to a state
Switching instantly will become an option of the animation
This means it can also be defined directly in a state itself:
Cycling through state
Next has been renamed to cycling, because next was pretty ambigious depending on the context.
Notice how we use an array of states names here, to support animation options as second argument
Special states
There are three special states that will be set automatically and can't be overridden:
layer.states.default
- The state the layer had upon creation.layer.states.previous
- The previous state the layer was inlayer.states.current
- The current state the layer is inThese states contain the actual values and not (as is the case with
layer.states.current
now) the state string. The name of the previous an current states will still be available throughlayer.states.previous.name
andlayer.states.current.name
. Thename
key only gets added in the context of these special states, so the special state objects are not the same as the actual state object. They are completely equal, with the name key added.Notice the absence of
layer.states.next
, this functionality will be provided bylayer.stateCycle()
as described above.Listing all the states
We will add a new property
layer.stateNames
that lists all the names of states currently defined on a layer. This list will containlayer.states.initial
, but won't containdefault
andcurrent
.The states on a layer is pretty much a normal object. So you can use different ways to loop through the states.
Older stuff:
Some refactoring for the new animation api
LayerStateMachine
.AnimationStart
,AnimationStop
andAnimationEnd
.StateWillSwitch
andStateDidSwitch
in favor ofStateSwitchStart
,StateSwitchStop
andStateSwitchEnd
.LayerStates
to use prototype properties, so we don't make them in constructor.Changes
.add
) to use as a state name, because they would not show up in state names.initial
state and keepdefault
as is.Worries
Object.keys(layer.states)
should be the same asfor k in layer.states
but that does not seem to be possible.layer.states.current
back from the name to the actual properties and add a name if you return them so you can dolayer.states.current.name
. This would lose comparison by reference, though.Layer
→LayerStates
→LayerStateMachine
and back. Not the end of the world, but I'm not sure if the GC is happy about this.See: https://github.com/motif/company/issues/2633, https://github.com/motif/company/issues/2635