Fixup updateConfig, add tests
ClosedPublic

Authored by skevy on Aug 10 2017, 4:44 AM.

Details

Summary

As mentioned in D3299, we had some weird flaws in updateConfig.

Essentially what this diff accomplishes is keeping the spring dynamics correct depending on what param is changed in the update.

E.g.:

  • A spring is initially configured to go from 50 - 100, with an initial velocity of 0.
  • Sometime during the animation, spring.updateConfig({ toValue: 200 }) is called.
  • Even though updateConfig restarts the animation, the spring should continue from where it left off (the position before updateConfig is called), and start with an initial velocity equivalent to the velocity in which the spring was traveling before updateConfig.

There are tests included here to prove this behavior. I added some other tests as well here to round out some of the coverage.

Also, I'm doing a driveby and adding removeListener and removeAllListeners...as this is something that's needed.

Test Plan

Run the tests!

Diff Detail

Repository
R51 wobble
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
skevy created this revision.Aug 10 2017, 4:44 AM
Restricted Application completed building Restricted Buildable.Aug 10 2017, 4:45 AM
skevy updated this revision to Diff 11389.Aug 10 2017, 4:48 AM
  • clearAllListeners -> removeAllListeners. Update comments/TS defs
Restricted Application completed building Restricted Buildable.Aug 10 2017, 4:50 AM
skevy updated this revision to Diff 11390.Aug 10 2017, 4:51 AM

Fix diff

skevy updated this revision to Diff 11391.Aug 10 2017, 4:52 AM

Fix diff again :(

Restricted Application completed building Restricted Buildable.Aug 10 2017, 4:53 AM
Restricted Application completed building Restricted Buildable.
src/__tests__/Springs-test.js
92

its

167

I think this is a leftover comment from the other test. "Change spring velocity mid-flight" would be more accurate, but I think the code speaks for itself.

176

first

src/index.js
137

In both places, its rather than it's

appsforartists accepted this revision.Aug 10 2017, 12:25 PM
This revision is now accepted and ready to land.Aug 10 2017, 12:25 PM
skevy updated this revision to Diff 11394.Aug 10 2017, 12:38 PM
  • Fix nits
Restricted Application completed building Restricted Buildable.Aug 10 2017, 12:40 PM
skevy updated this revision to Diff 11395.Aug 10 2017, 12:41 PM

Fix diff?

Restricted Application completed building Restricted Buildable.Aug 10 2017, 12:42 PM
This revision was automatically updated to reflect the committed changes.