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: fill up the missing jsdoc for classes and series types #8307

Closed
wants to merge 2 commits into from

Conversation

e-cloud
Copy link

@e-cloud e-cloud commented May 9, 2018

add these docs is to generate highcharts.d.ts at the next step.

Seperating the changes into two PR is good for reviewing.

partly fix #4876

@TorsteinHonsi could you have a review over this.

Note: all of changes only involve comment improvement.

@e-cloud
Copy link
Author

e-cloud commented May 14, 2018

@TorsteinHonsi could you have a look?

@scott-ho
Copy link

@oysteinmoseng could you have a review? It seems good.

@TorsteinHonsi
Copy link
Collaborator

Thanks for the PR!

Our main purpose of the JSDoc doclets is to generate our class reference and options reference. The existing doclets that show up in the class reference are carefully revised to be informative to our users. Unfortunately, the remaining code is not revised. So exposing those doclets results in some inconsistencies and errors in the generated documentation, plus there are internal functions that we don't want to document because they may change in the future etc.

skjermbilde 2018-05-15 kl 14 24 42

@e-cloud e-cloud force-pushed the type-tree branch 2 times, most recently from b38a0b0 to 25079f2 Compare May 15, 2018 17:44
@e-cloud
Copy link
Author

e-cloud commented May 17, 2018

@TorsteinHonsi you are right. Then I make a plugin to filter out those accidentally exposed docs.

could you have a review again? The extra docs must be filtered out now.

@e-cloud
Copy link
Author

e-cloud commented May 26, 2018

It has been a while waiting for reviewing.

I've push another branch(https://github.com/e-cloud/highcharts/tree/ref) to show how i generate the highcharts.d.ts. It not ready yet for tsd-jsdoc not being published.

No offense, however, @TorsteinHonsi if your team don't want to accept the PR, please state it and close the PR, I will make another PR to DefinitelyTyped.

@oysteinmoseng
Copy link
Member

Torstein is on vacation at the moment, but let's have a look.

@cvasseng Is this something we can pull in?

@bre1470 bre1470 requested review from bre1470 and TorsteinHonsi and removed request for bre1470 June 8, 2018 11:08
@TorsteinHonsi
Copy link
Collaborator

Thanks again for your contribution @e-cloud !

We are now continuing the work on our own TypeScript support, and will take this PR into account.

@e-cloud
Copy link
Author

e-cloud commented Jun 9, 2018

sounds good. @TorsteinHonsi did you have a look over the ref branch? a highcharts.d.td is auto generated. Although it is not perfect yet. But we can continue to improve jsdoc or improve the types directly.

BTW, could you reveal the plan of our own typescript support

@bre1470 bre1470 self-assigned this Jun 11, 2018
@bre1470
Copy link
Contributor

bre1470 commented Jun 11, 2018

@e-cloud
First of all I like to thank you for the work, you have done here. We are still reviewing your massive changes and will probably integrate part of it into Highcharts. Unfortunately there are still some unresolved issues, so for now everything is in-between. We will integrate the pull request at the end of this.

We are working internally since last year on the problem (with lower priority until recently). The goal beside declarations is to synchronize the online documentation and the declarations for TypeScript. Thanks to your contribution we might speed this up and hopefully have working declarations in the near future. We then will integrate it in our node package and also provide it to the definitelyTyped project to replace the current declarations there.

@e-cloud
Copy link
Author

e-cloud commented Jun 11, 2018

@sophiebremer Could you point out the unresolved issues to see if I could solve them?

@bre1470
Copy link
Contributor

bre1470 commented Jun 12, 2018

@e-cloud No, I am sorry. These issues are about (project) synchronization.

@e-cloud
Copy link
Author

e-cloud commented Jun 13, 2018

@sophiebremer what do you mean by synchronization? If you mean branch synchronization, I've rebase the branch to latest master two days ago.

@TorsteinHonsi
Copy link
Collaborator

Closing this PR now, we're on track to complete TypeScript support.

@highcharts highcharts locked as resolved and limited conversation to collaborators Jul 29, 2019
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.

Typescript .d.ts
5 participants