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

feat(operators): operator browser page #34

Merged
merged 37 commits into from
Oct 21, 2017
Merged

feat(operators): operator browser page #34

merged 37 commits into from
Oct 21, 2017

Conversation

btroncone
Copy link
Collaborator

@btroncone btroncone commented Oct 11, 2017

This includes the first version of UI for the operator section, as well as beginning samples for combineAll and combineLatest.

The static operator content is split into individual files, I figured this would be easiest for contributors to consume, or it can be extracted into a document db or similar if we choose to go that route in the future. I'm currently working on combining the content and examples from the official docs with relevant examples from learnrxjs.

Please let me know if you have any additional thoughts or ideas, thanks!

Demo

@ladyleet
Copy link
Member

@btroncone awesome! @kwonoj @benlesh can y'all peep at this when you get a chance?

@@ -0,0 +1,65 @@
import { OperatorDoc } from '../operator.model';

export const combineLatest: OperatorDoc = {
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this whole document is a stub and we're going to be generating this data from the TS files directly, right?

We should be able to link the exact line (and SHA) in Github this doc was generated from so people can "read the source"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I wasn't sure about the long term solution. This is just a temporary hack so I could start getting data on the page / establish structure.

@ladyleet
Copy link
Member

@benlesh let's worry about that as we come to it but just get these operators and the official docs so that people can start having a good reference let's worry about that as we come to it but just get these operators and the official docs so that people can start having a good reference. The rest can be incremental.

@kwonoj
Copy link
Member

kwonoj commented Oct 17, 2017

I'm feeling PR grows too large that nearly not able to review. What's plan's here? Originally it says This isn't ready to merge - is this means PR will grow further, or this is just poc and will breakdown into separate PR as small chunks?

@btroncone
Copy link
Collaborator Author

I think where this stands now is a good base to build off for the operator UI. Operator content and additional UI tweaks can be included in future (much smaller) PR's.

@kwonoj
Copy link
Member

kwonoj commented Oct 17, 2017

@btroncone my Q is about this PR itself. it have more than 130 files in single PR, I'm not sure I can follow all of them.

@ladyleet
Copy link
Member

Can we just yolo this because it's one of the larger more significant PR is and it's not like anything is going to break?

@ladyleet ladyleet closed this Oct 17, 2017
@ladyleet ladyleet reopened this Oct 17, 2017
@ladyleet
Copy link
Member

oops sorry fat fingers :-)

@btroncone
Copy link
Collaborator Author

@kwonoj Sure, you can pretty much ignore everything from operator-docs, those are just placeholders so I could start getting a sense of structure for operator content and build the menu.

The rest make up the operator browser page / module. I split up each section of each operator into it's own component (ex. operator-header, operator-examples, etc) so they would be easier to maintain and extend. Most are just small presentation components, the only component with much going on is the one routable component operators.component.

The other new files included are for the rx-marbles web component.

Hope this helps, sorry for the massive amount of files. Since this is one big page I wanted to get it to a working state before submitting, my PRs will be much smaller moving forward.

@kwonoj
Copy link
Member

kwonoj commented Oct 17, 2017

I still believe splitting this PR into smaller chunks will be helpful, and do not add more stuff into current. First, at least placeholder (/transformation..) can be separated, and others as well like rxMarble integration. And at least main core component needs some test (..I guess? we need test coverage, right?).

@btroncone
Copy link
Collaborator Author

Sure, I'll add some tests for the core component.

@btroncone
Copy link
Collaborator Author

@kwonoj It looks like you are missing a - in prod build, it should be ng build --prod. Noticed in the travis build.

- ng build -prod

@kwonoj
Copy link
Member

kwonoj commented Oct 17, 2017

@btroncone yeah, good catch.

@btroncone btroncone changed the title Operator browser feat(operators): operator browser page Oct 19, 2017
@ladyleet
Copy link
Member

@btroncone what did we decide on getting this PR in or breaking it into smaller ones or.... <3 thanks for all your hard work!

@btroncone
Copy link
Collaborator Author

@ladyleet I think it makes sense to go in as one PR as it's really just one page, everything here fits together. The file creep was caused by the 70+ operator placeholder files and 3 files (ts, html, scss) for each presentation component within each operator.

I'm pretty much tapped for the weekend so if you need me to split this up I'll have to do it early next week. Let me know what you think, thanks! 👍

@ladyleet
Copy link
Member

@btroncone meh, let's just merge it. it's out of date with base - can you fix that? no rush if it needs to wait for next week.

@btroncone
Copy link
Collaborator Author

Sure, done. 👍

@ladyleet
Copy link
Member

@btroncone oops looks like travis build failed for some reason

Split descriptions for combineAll and combineLatest between two lines to satisfy lint requirement
(line +140 chars)
@btroncone
Copy link
Collaborator Author

Sorry, it was linting issue. All checks should be passing now. :)

@ladyleet
Copy link
Member

omg! merging! woot!

@ladyleet ladyleet merged commit 73a724e into ReactiveX:master Oct 21, 2017
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.

5 participants