-
Notifications
You must be signed in to change notification settings - Fork 62
docs(operators): add documentation for windowToggle #246
Conversation
Codecov Report
@@ Coverage Diff @@
## master #246 +/- ##
=======================================
Coverage 90.49% 90.49%
=======================================
Files 115 115
Lines 442 442
Branches 10 10
=======================================
Hits 400 400
Misses 40 40
Partials 2 2
Continue to review full report at Codecov.
|
name: 'windowToggle', | ||
operatorType: 'transformation', | ||
signature: | ||
'public windowToggle(openings: Observable<O>, closingSelector: function(value: O): Observable): Observable<Observable<T>>', |
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.
In contrast with the old documentation, we don't want any "generics" in the method signature as per #196. So please remove the generics.
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.
Okay, will do it
i % 2 ? Rx.Observable.interval(500) : Rx.Observable.empty() | ||
).mergeAll(); | ||
result.subscribe(x => console.log(x)); | ||
`, |
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.
Add some expected console output as a comment below the subscription in the code. Also add it in the jsbin example.
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.
Got it.
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.
Rest looks pretty good to me. Good Job !!!
parameters: [ | ||
{ | ||
name: 'openings', | ||
type: 'Observable<O>', |
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.
maybe remove this generic. Could be confusing?!
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.
hmm, will remove it.
|
||
const clicks = fromEvent(document, 'click'); | ||
const openings = interval(1000); | ||
const result = clicks.windowToggle(openings, i => |
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 should be using .pipe
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.
Okay, will do it.
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
Close #126