[refactored] _tap to use named args
ClosedPublic

Authored by appsforartists on Oct 6 2017, 11:51 PM.

Diff Detail

Repository
R13 material-motion/material-motion-js
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
appsforartists created this revision.Oct 6 2017, 11:51 PM
Restricted Application added a reviewer: O2: Material Motion. · View Herald TranscriptOct 6 2017, 11:51 PM
Restricted Application added a project: Material Motion. · View Herald Transcript
Restricted Application added a reviewer: O3: Material JavaScript platform reviewers. · View Herald Transcript
Restricted Application added a reviewer: Material Motion. · View Herald Transcript
Restricted Application completed building Restricted Buildable.Oct 6 2017, 11:55 PM
vietanh requested changes to this revision.Oct 10 2017, 7:53 PM
vietanh added a subscriber: vietanh.
vietanh added inline comments.
packages/core/src/operators/foundation/__tests__/_remember.test.ts
86

nit: do we enforce 80 char linebreak?

packages/core/src/operators/foundation/_reactiveMap.ts
48

Do we have to swap D and T for _ReactiveMapArgs here?

This revision now requires changes to proceed.Oct 10 2017, 7:53 PM
appsforartists added inline comments.Oct 10 2017, 8:12 PM
packages/core/src/operators/foundation/__tests__/_remember.test.ts
86

I don't have a formatting strategy yet. Will probably add clang-format or prettier at some point, but haven't gotten there yet.

packages/core/src/operators/foundation/_reactiveMap.ts
48

I'm not sure I follow. On lines 30 and 48, I see <D, T, U> in both.

vietanh accepted this revision.Oct 10 2017, 8:25 PM
vietanh added inline comments.
packages/core/src/operators/foundation/__tests__/_remember.test.ts
86

ACK

packages/core/src/operators/foundation/_reactiveMap.ts
48

I see, I didn't expand the code and didn't see the definition on line 30.

Another reason I had this comment because in all your other files, you have swapped the 2 generic types T and D, such as _ReactiveNextOperatorArgs in _reactiveNextOperator.ts or Operation<T, D> in types.ts but not here. Therefore, I want to doublecheck if this is as you intended.

This revision is now accepted and ready to land.Oct 10 2017, 8:25 PM

Reordered _ReactiveMapArgs to match _ReactiveNextOperatorArgs.

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