Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(from): Convert docs for from #4031

Merged
merged 2 commits into from Aug 21, 2018
Merged

Conversation

pertrai1
Copy link
Contributor

Description:

Update the new docs for from with the older docs and add live StackBlitz example.

@coveralls
Copy link

coveralls commented Aug 17, 2018

Coverage Status

Coverage decreased (-0.2%) to 96.781% when pulling c996b0a on pertrai1:from-docs-update into 030eee7 on ReactiveX:master.

Copy link
Member

@niklas-wortmann niklas-wortmann left a comment

Choose a reason for hiding this comment

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

Thank you very much for this awesome pull request. I requested some changes. If anything is not completly clear, feel free to ask me at any time

@@ -34,3 +34,74 @@ export function from<T>(input: ObservableInput<T>, scheduler?: SchedulerLike): O

throw new TypeError((input !== null && typeof input || input) + ' is not observable');
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

Hi Rob, basically this is an awesome pr. Thanks for all the work you put into it. But this part has to be above the function declaration to be valid JSDoc. Maybe take a look at another operator like fromEvent then you will see what I mean.

* // Logs:
* // 10 20 30
* ```
* <a href="https://stackblitz.com/edit/js-tfvhag" target+"_blank">Stackblitz example</a>
Copy link
Member

Choose a reason for hiding this comment

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

I really appreciate adding stackblitz, but first I think it doesn't really adds value for the small api examples. I think it would be more helpful for the tutorial, we want to add soon. Additionally this messes up the outline in the docs and it isn't conform with accessibility standards. Really thanks for this, but I think this makes sense to add in a seperate pull request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds great to me. I will remove these examples and use later in tutorials.

* ```
*
* <a href="https://stackblitz.com/edit/js-kghdtb" target="_blank">StackBlitz example</a>
*/
Copy link
Member

Choose a reason for hiding this comment

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

could you add param describtion for the method signature and return value

@niklas-wortmann
Copy link
Member

BTW: This will close #4027

@pertrai1 pertrai1 force-pushed the from-docs-update branch 3 times, most recently from 6213d4b to ae81c6a Compare August 18, 2018 13:46
Copy link
Member

@niklas-wortmann niklas-wortmann left a comment

Choose a reason for hiding this comment

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

Just one really minor change. After this it is very good in my opinion

* @see {@link fromEventPattern}
* @see {@link fromPromise}
*
* @param {ArrayLike<T> | ObservableInput<T>} A subscription object, a Promise, an Observable-like,
Copy link
Member

Choose a reason for hiding this comment

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

ObserableInput is a type of SubscribableOrPromise<T> | ArrayLike<T> | Iterable<T> therefore adding ArrayLike<T> isn't really needed

@cartant cartant merged commit 040ec2f into ReactiveX:master Aug 21, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 20, 2018
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.

None yet

4 participants