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

Conversation

@natmegs
Copy link
Contributor

@natmegs natmegs commented Dec 2, 2017

Add initial version of documentation for transformation operator 'scan'

closes #122

Add initial version of documentation for transformation operator 'scan'

closes ReactiveX#122
@rxjs-bot
Copy link

rxjs-bot commented Dec 2, 2017

Warnings
⚠️

❗ Big PR (1)

(1) : Pull Request size seems relatively large. If Pull Request contains multiple changes, split each into separate PR will helps faster, easier review.

Generated by 🚫 dangerJS

@codecov-io
Copy link

codecov-io commented Dec 2, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #175   +/-   ##
=======================================
  Coverage   74.78%   74.78%           
=======================================
  Files           9        9           
  Lines         119      119           
  Branches        6        6           
=======================================
  Hits           89       89           
  Misses         30       30

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 8804ed5...689b95d. Read the comment docs.

@natmegs
Copy link
Contributor Author

natmegs commented Dec 2, 2017

Hmm, not sure why all the package-lock.json is showing up as a change.... The only file I changed was the scan.ts file. Anyone know why this would be happening?

Copy link
Collaborator

@btroncone btroncone left a comment

Choose a reason for hiding this comment

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

Really awesome work, thanks! Just a few small tweaks. 👍

<p>
Combines together all values emitted on the source, using an accumulator
function that knows how to join a new source value into the accumulation from
the past. Is similar to <span class="markdown-code">reduce</span>, but emits the intermediate accumulations.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you turn this into a link to reduce?

description: 'The initial accumulation value.'
}
],
marbleUrl: 'http://reactivex.io/rxjs/img/scan.png',
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like scan has an interactive marble diagram available (http://rxmarbles.com/#scan). Could you set useInteractiveMarbles property to true? This will use that on the page rather than the static image.

@ashwin-sureshkumar
Copy link
Collaborator

@natmegs - What version of node and npm are you using just trying to look what would have caused the package lock generation

@natmegs
Copy link
Contributor Author

natmegs commented Dec 8, 2017

Thanks for the feedback and sorry for the delay. I added the changes but as far as I can tell there is no page set up for reduce, so that link won't work until that page is created.

@ladyleet
Copy link
Member

ladyleet commented Dec 8, 2017

thx @natmegs!

@ashwin-sureshkumar
Copy link
Collaborator

@btroncone - please re-review when you get a chance.

Copy link
Member

@ladyleet ladyleet left a comment

Choose a reason for hiding this comment

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

Great job Nat! Loved this - I am reviewing with Ben so... he's a little harder on the docs than everyone else. ;) But, it's also great! <3

let ones = clicks.mapTo(1);
let seed = 0;
let count = ones.scan((acc, one) => acc + one, seed);
count.subscribe(x => console.log(x));
Copy link
Member

Choose a reason for hiding this comment

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

  • Should use const vs let
  • Use ES6 style imports
  • Use pipeable operators
  • Use rxjs/observable/fromEvent

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ladyleet Thanks for the feedback! To make sure I've understood this correctly before I commit, should it look like this?

const clicks = Rx.Observable.fromEvent(document, 'click');
const ones = clicks.mapTo(1);
const seed = 0;
const count = ones.pipe(scan((acc, one) => acc + one, seed));
count.subscribe(x => console.log(x));

@benlesh benlesh merged commit 689b95d into ReactiveX:master Dec 19, 2017
@benlesh
Copy link
Member

benlesh commented Dec 19, 2017

I've resolved the conflicts and merged. Thanks, @natmegs!!

(@ladyleet, @btroncone, et al: we should probably not be committing the package-lock.json for this project, it seems to be a source of merge pain more than it helps) (nothing to do with you, @natmegs).

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 : transformation - scan

8 participants