-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add JSDoc comments for additional PaperScope methods #1818
base: develop
Are you sure you want to change the base?
Conversation
Great, thank you @lazerwalker! Could you make it wrap at 80 chars? All code does in paper.js And I have a feeling there is a better way to document the two separate version of the method signature. @sasensi since TS is your area, what do you think? I think two separate declarations would be better, yes? |
@lehni, yes indeed, I think that this would be more consistent with the rest of the codebase. |
Just pushed a change hard-wrapping it to 80 chars. In an ideal world, the TS signature would look something like I couldn't easily find a way to generate anything looking like that via this JSDoc setup. Trying to include in a full object shape like that in JSDoc triggered gulp build errors (at least, the way I was doing it), and creating a dedicated separate interface/type felt maybe a little heavy for this specific situation. It felt to me like merely describing the shape of the object in plaintext was an decent compromise in clarity vs verbosity, but totally open to suggestion. I couldn't find any particularly good examples of something similar already in the codebase, but maybe you have a good one to point to? |
@lazerwalker, indeed, we don't support yet deep object description so Lines 75 to 117 in b081fd7
|
Just split it up into two blocks! Let me know if there are any other issues. Also happy to squash into a single commit before merging if it's at that point + you'd prefer. |
8e6310e
to
17e6be0
Compare
* Returns a valid HTML canvas you can use. | ||
* Internally, this may reuse an existing canvas. | ||
* | ||
* @param {Object} options An object containing the canvas width / height |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a Size
object really, and the param name should be size
then.
* Internally, this may reuse an existing canvas. | ||
* | ||
* @param {Object} options An object containing the canvas width / height | ||
* | ||
* @option width {Number} | ||
* @option height {Number} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using @option, it should include the object param name as well, so options.width
and options.height
in your use case. But since it should really be a Size object, this isn't even needed.
39b63a1
to
b90a711
Compare
4a6bf6f
to
0f4afc5
Compare
Description
The TypeScript type definitions don't include
createCanvas
orclear
on the PaperScope object. This PR adds JSDoc comments for those two functions so they show up in both the online documentation and the generated.d.ts
typings files.This is a comment/documentation-only change.
I've manually confirmed that both the docs and .d.ts file look sensible after running
gulp docs
locally with these changes.Thanks! :D
Checklist
npm run jshint
passes)