-
Notifications
You must be signed in to change notification settings - Fork 62
docs(operators): add documentation for sample & sampleTime #207
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #207 +/- ##
==========================================
+ Coverage 90.78% 90.82% +0.04%
==========================================
Files 116 117 +1
Lines 445 447 +2
Branches 9 9
==========================================
+ Hits 404 406 +2
Misses 40 40
Partials 1 1
Continue to review full report at Codecov.
|
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.
Looks great, just one small change. Thanks for the PR! 👍
extras: [ | ||
{ | ||
type: 'Tip', | ||
text: `It's like sampleTime, but samples whenever the notifier Observable emits something.` |
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.
Can we add a link here to sampleTime
?
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.
Sure, i will add it.
`, | ||
externalLink: { | ||
platform: 'JSBin', | ||
url: 'http://jsbin.com/xapiviz/edit?js,console,output' |
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.
Please use embedded instead of edit.
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.
Oops! Yeah of course.
`, | ||
externalLink: { | ||
platform: 'JSBin', | ||
url: 'http://jsbin.com/hohulon/edit?js,console,output' |
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.
same here
@hardikpthv - awesome job !! Please make sure there is a single operator per PR. Easier to organize and track. Also there is already a PR for sample. I will close it. Let's make sure we don't repeat work :) |
Oh i see! @ashwin-sureshkumar I will keep it in mind. :) |
@sumitarora @btroncone - Can you please re-review this PR? |
# Conflicts: # src/operator-docs/filtering/sample.ts
# Conflicts: # src/app/team/team.service.ts
@btroncone @sumitarora - please re-review when get a chance |
@btroncone @sumitarora - please re-review when get a chance |
@btroncone @sumitarora - please re-review when get a chance |
name: 'sampleTime', | ||
operatorType: 'filtering', | ||
signature: | ||
'public sampleTime(period: number, scheduler: Scheduler): 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.
Remove generic
{ | ||
name: 'Every second, emit the most recent click at most once', | ||
code: ` | ||
const clicks = Rx.Observable.fromEvent(document, 'click'); |
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.
Update it to ES6 imports
code: ` | ||
const clicks = Rx.Observable.fromEvent(document, 'click'); | ||
const result = clicks.sampleTime(1000); | ||
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 expected output
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.
@hardikpthv I'm a bit confused :D will you take care of your comments in this pr :D
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.
Sure @jwo719 It is for my reference :D
I think sample operator can be ignored as it's already there. |
can this be merged @ashwin-sureshkumar or any changes expected? |
Closes: #192