docs(groupBy): fix examples #4554
Conversation
|
Oops, just realised #4554 addresses some of these issues but not all. Happy to re-jig my changes with that other PR once it's merged, if that's easier. |
Pull Request Test Coverage Report for Build 8186
💛 - Coveralls |
|
Unfortunately there are some merge conflicts in the meantime. @tanem could you resolve the conflicts? |
|
Ah ok, no worries @jwo719, will get onto that ASAP 💯 |
tanem
force-pushed
the
fix-group-by-examples
branch
from
6c21d06
to
7c3a6c9
Compare
|
Hey @jwo719, have resolved the merge conflicts 👍 |
src/internal/operators/groupBy.ts
Outdated
| * {id: 1, name: 'JavaScript'}, | ||
| * {id: 2, name: 'Parcel'}, | ||
| * {id: 2, name: 'webpack'}, | ||
| * {id: 1, name: 'TypeScript'} | ||
| * {id: 1, name: 'TypeScript'}, | ||
| * {id: 3, name: 'TSLint'} | ||
| * ).pipe( | ||
| * groupBy(p => p.id, p => p.name), | ||
| * mergeMap( (group$) => group$.pipe(reduce((acc, cur) => [...acc, cur], ["" + group$.key]))), |
There was a problem hiding this comment.
Not really part of the PR but I noticed this extra space mergeMap( (group$).... Can we get it removed?
And one more thing. Is there any reason why group$.key is send as a string and later parsed to an integer? If the string is required, does it make sense then to make it [`${group$.key}`] instead?
There was a problem hiding this comment.
Thanks for the feedback @dzhavat, made both of those changes 👍
There was a problem hiding this comment.
Looks better 👍
I'm actually not part of the RxJS team, so I commented more as a fellow contributor rather than someone who is responsible for reviewing the PR :)
Fixes a few glitches so the examples are runnable. This also includes stripping the type definitions since the example code blocks specify `javascript`, and the other examples I've seen so far don't seem to include these.
tanem
force-pushed
the
fix-group-by-examples
branch
from
7c3a6c9
to
6c07f7a
Compare
Fixes a few glitches so the examples are runnable.
This also includes stripping the type definitions since the example code blocks specify
javascript, and the other examples I've seen so far don't seem to include these.