Skip to content
This repository has been archived by the owner on Oct 1, 2018. It is now read-only.

docs(operators): add documentation for operator do #167

Merged
merged 7 commits into from
Nov 28, 2017

Conversation

mustafamg
Copy link
Contributor

@mustafamg mustafamg commented Nov 23, 2017

closes #131

@codecov-io
Copy link

codecov-io commented Nov 23, 2017

Codecov Report

Merging #167 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #167   +/-   ##
=======================================
  Coverage   85.71%   85.71%           
=======================================
  Files           8        8           
  Lines          91       91           
  Branches        4        4           
=======================================
  Hits           78       78           
  Misses         13       13

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed1390...5d2eb98. Read the comment docs.

name: 'do',
operatorType: 'utility',
signature:
'public do(nextOrObserver: Observer | function, error: function, complete: function): Observable',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could mark the optional parameters with a question mark:

public do(nextOrObserver?: Observer | function, error?: function, complete?: function): Observable

Copy link
Contributor Author

@mustafamg mustafamg Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you it will be better to have a distinctive mark for optional parameters. My only concern is that this is inconsistent with the rest of the document. I think you should make this as a suggestion for the full documentation, then all contributors can follow it as a rule.
Right now, I don't think I should be the only one making this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened #168 which encompass this suggestion with others.

},
walkthrough: {
description: `
<p><code>do</code> Returns a mirrored Observable of the source Observable,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returns (without the R)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, will do that.

@ladyleet
Copy link
Member

Thank you @mustafamg for your contribs and thank you @feloy for your code review! :) @sumitarora @ashwin-sureshkumar @btroncone if any of y'all have time to review this!

`,
externalLink: {
platform: 'JSBin',
url: 'http://jsbin.com/mikiqub/edit?js,console,output'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one small change. Instead of logging whole event in js bin which outputs the whole event details in jsbin.

[object MouseEvent] {
  altKey: false,
  AT_TARGET: 2,
  bubbles: true,
  BUBBLING_PHASE: 3,
  button: 0,
  buttons: 0,
  cancelable: true,
  cancelBubble: false,
  CAPTURING_PHASE: 1,
  clientX: 182,
  clientY: 292,
  composed: true,
  composedPath: function composedPath() { [native code] },
  ctrlKey: false,
  currentTarget: [object HTMLDocument] {
    activeElement: [object HTMLBodyElement] { ... },
    addEventListener: function addEventListener() { [native code] },
    adoptNode: function adoptNode() { [native code] },

Could you just do event type. .do(ev => console.log(ev)) to .do(ev => console.log(ev.type))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ashwin-sureshkumar ashwin-sureshkumar merged commit 7f3ce3c into ReactiveX:master Nov 28, 2017
@mustafamg mustafamg deleted the doc-do-branch branch December 1, 2017 12:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add docs for operator : utility - do
6 participants