-
Notifications
You must be signed in to change notification settings - Fork 62
docs(operators): Add documentation for fromEvent #286
base: master
Are you sure you want to change the base?
Conversation
This adds documentation for the fromEvent operator. The documentation has been partially rewritten to clarify certain parts a bit more. This change will close issue ReactiveX#78
There were still a couple of examples necessary for the fromEvent operator. These have now been added
Codecov Report
@@ Coverage Diff @@
## master #286 +/- ##
=======================================
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.
|
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.
I have added some comments. But I definetly need a closer look for a complete review.
name: 'fromEvent', | ||
operatorType: 'creation', | ||
signature: `static fromEvent(target, | ||
eventName, options, selector): Observable`, |
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.
nice try :D just kidding. Could you add the types as long as #269 is in discussion
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.
Ha! Apparently I wasn't paying attention, yes they should be there :)
</p> | ||
|
||
<p> | ||
Note that if the event target would normally (without RxJS) call the registered event handler with more than one argument, |
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 add an exampe event here
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.
I've added an example just below this paragraph. I think it makes more sense to put it there (so it doesn't stop the flow of the text with an 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.
Just a thought, I would move the jquery example below the supported event types. Since its an additional note and breaks the flow of thought of fromEvent
…handler with more than one argu Added a small jQuery example that creates and triggers an event handler with more than one argument.
</p> | ||
|
||
<p> | ||
Note that if the event target would normally (without RxJS) call the registered event handler with more than one argument, |
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.
Just a thought, I would move the jquery example below the supported event types. Since its an additional note and breaks the flow of thought of fromEvent
The example of a non-one argument event handler, written with jQuery was breaking the flow ot the text. Moved it below the event targets and added a small reference in the source text to continue reading below.
This adds documentation for the fromEvent operator. The documentation has been partially rewritten
to clarify certain parts a bit more. This change will close issue #78. Also added an extra innerHTML property binding for the parameter.description to be able to use a hyperlink.