Make API clearer re: when spring is at rest
ClosedPublic

Authored by skevy on Aug 14 2017, 7:25 PM.

Details

Summary

The public API was confusing with regards to when the spring at rest, and didn't differentiate between "is animating" and "is at rest".

Changes overview:

  • isAtRest and isAnimating are public getters
  • onActive -> onStart, onAtRest -> onStop
  • Added test for stop/onStop
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 14 2017, 7:25 PM
Restricted Application completed building Restricted Buildable.Aug 14 2017, 7:27 PM
appsforartists added inline comments.Aug 14 2017, 7:39 PM
src/__tests__/Springs-test.js
200

"that the spring has stopped when .stop() is called mid-simulation"

209

Is toBe too accurate - will this be a flaky test?

215

I think you meant to assert this after calling stop().

src/index.js
69

Why are you typing (and explicitly returning from) a constructor?

133

See the comment edits in the .d.ts

wobble.d.ts
36

If the spring has reached its toValue, or if its velocity is below the restVelocityThreshold, it is considered at rest. If stop is called during a simulation, both isAnimatingand isAtRest will be false.

41

Note: this is distinct from whether or not it is at rest. See also isAtRest.

skevy updated this revision to Diff 11408.Aug 14 2017, 7:56 PM
skevy marked 7 inline comments as done.

Fix nits

src/index.js
69

Accident. Was trying to fix a Flow issue, forgot to remove.

Restricted Application completed building Restricted Buildable.Aug 14 2017, 7:58 PM
appsforartists accepted this revision.Aug 14 2017, 8:01 PM
This revision is now accepted and ready to land.Aug 14 2017, 8:01 PM
appsforartists added inline comments.Aug 14 2017, 8:06 PM
wobble.d.ts
41

Sorry, I was unclear here. I think you should still explain what it does, but also include the note:

Whether or not the spring is currently emitting values.

Note: this is distinct from whether or not it is at rest.   See also `isAtRest`.
50

I should have included () in stop(). Sorry bout that.

skevy marked 2 inline comments as done.Aug 14 2017, 8:08 PM
skevy updated this revision to Diff 11409.Aug 14 2017, 8:08 PM

Fix more nits!

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