Add Pinchable, Rotatable and DirectlyManipulable plans
ClosedPublic

Authored by rcameron on Sep 13 2016, 7:28 PM.

Diff Detail

Repository
rMDMDIRECTMANIPULATIONSWIFT material-motion/direct-manipulation-swift
Branch
new-plans (branched from develop)
Lint
Lint OK
Unit
Unit Tests OK
rcameron retitled this revision from to Add Pinchable, Rotatable and DirectlyManipulable plans closes #13, #12, #11, #9.Sep 13 2016, 7:28 PM
rcameron updated this object.
rcameron edited the test plan for this revision. (Show Details)
Restricted Application added a project: Material Motion. · View Herald Transcript
rcameron retitled this revision from Add Pinchable, Rotatable and DirectlyManipulable plans closes #13, #12, #11, #9 to Add Pinchable, Rotatable and DirectlyManipulable plans.Sep 13 2016, 7:31 PM
rcameron updated this object.
rcameron updated this object.
rcameron updated this revision to Diff 6701.Sep 13 2016, 7:33 PM
  • copyright
featherless requested changes to this revision.Sep 13 2016, 7:40 PM
featherless added a reviewer: featherless.
featherless added a subscriber: featherless.
featherless added inline comments.
src/DirectManipulationMotionFamily.swift
20

Plan documentation should describe the expected contract of fulfillment, not necessarily how it's implemented.

79

Ditto here and throughout.

234

Place this just above func set(transactionEmitter: TransactionEmitting).

242

Remove.

262

We may want to add instructions to the DirectlyManipulable plan's docs explaining how to configure the gesture recognizer delegates to handle multiple gestures simultaneously in case they've already set a delegate.

274

We can only safely say that simultaneous recognition should occur when it's one of our three gesture recognizers, so we likely want to test that here.

tests/unit/ComposedGestureTests.swift
28

Is it possible to compile code with a nil gesture recognizer?

tests/unit/SimpleGestureTests.swift
34

Writing tests like so may improve clarity:

let scheduler = Scheduler()

let transaction = Transaction()
transaction.add(plan: draggable, to: view)
scheduler.commit(transaction: transaction)
This revision now requires changes to proceed.Sep 13 2016, 7:40 PM
rcameron updated this revision to Diff 6702.Sep 13 2016, 7:58 PM
  • remove unnecessary composed plan test
  • clean up
rcameron marked 5 inline comments as done.Sep 13 2016, 8:00 PM
rcameron added inline comments.
src/DirectManipulationMotionFamily.swift
20

I took another shot at this.

tests/unit/ComposedGestureTests.swift
28

Good call. It's impossible to get into that situation.

featherless requested changes to this revision.Sep 13 2016, 8:04 PM
featherless added inline comments.
src/DirectManipulationMotionFamily.swift
20

Better.

I think we can drop the "Plans can be attached via a Transaction object." because hopefully we've taught that elsewhere.

99

stylenit: Documentation comments are wrapped on 100 characters. Was this wrapping intentional?

260

Can loop over the gestureRecognizers array now here.

267

And can loop over gestureRecognizers here as well.

tests/unit/ComposedGestureTests.swift
19

What did we need @testable for here?

This revision now requires changes to proceed.Sep 13 2016, 8:04 PM
featherless added inline comments.Sep 13 2016, 8:06 PM
src/DirectManipulationMotionFamily.swift
273

We expect there to be an emitter here, so emitter! would be a more accurate statement.

rcameron updated this revision to Diff 6703.Sep 13 2016, 8:25 PM
rcameron marked 3 inline comments as done.
  • more cleanup
src/DirectManipulationMotionFamily.swift
260

I left this as-is because I didn't see a way to simplify accessing the recognizer from the plan or vice-versa.

273

I changed the emitter type to be an implicitly unwrapped optional instead.

tests/unit/ComposedGestureTests.swift
19

Oops! Got in the zone of blindly adding @testable. Removing

rcameron marked 4 inline comments as done.Sep 13 2016, 8:26 PM
featherless accepted this revision.Sep 13 2016, 8:30 PM

Sweet!

src/DirectManipulationMotionFamily.swift
260

Oh! Good point. I misread the code there.

This revision is now accepted and ready to land.Sep 13 2016, 8:30 PM
rcameron closed this revision.Sep 13 2016, 8:47 PM