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

Add JSDoc comments for additional PaperScope methods #1818

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

lazerwalker
Copy link

@lazerwalker lazerwalker commented Jun 3, 2020

Description

The TypeScript type definitions don't include createCanvas or clear 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

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the JSHint rules (npm run jshint passes)

@lehni
Copy link
Member

lehni commented Jun 4, 2020

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?

@sasensi
Copy link
Contributor

sasensi commented Jun 4, 2020

@lehni, yes indeed, I think that this would be more consistent with the rest of the codebase.

@lazerwalker
Copy link
Author

lazerwalker commented Jun 4, 2020

Just pushed a change hard-wrapping it to 80 chars.

In an ideal world, the TS signature would look something like createCanvas(width: number | {width: number, height: number}, height?: number).

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?

@sasensi
Copy link
Contributor

sasensi commented Jun 4, 2020

@lazerwalker, indeed, we don't support yet deep object description so Object type is the best we can provide for now.
About splitting the documentation into 2 signatures, here is an example of how we do this:

/**
* Creates a new empty raster of the given size, and places it in the
* active layer.
*
* @name Raster#initialize
* @param {Size} size the size of the raster
* @param {Point} [position] the center position at which the raster item is
* placed
*
* @example {@paperscript height=150}
* // Creating an empty raster and fill it with random pixels:
* var width = 100;
* var height = 100;
*
* // Create an empty raster placed at view center.
* var raster = new Raster(new Size(width, height), view.center);
*
* // For all of its pixels...
* for (var i = 0; i < width; i++) {
* for (var j = 0; j < height; j++) {
* // ...set a random color.
* raster.setPixel(i, j, Color.random());
* }
* }
*/
/**
* Creates a new raster from an object description, and places it in the
* active layer.
*
* @name Raster#initialize
* @param {Object} object an object containing properties to be set on the
* raster
*
* @example {@paperscript height=300}
* var raster = new Raster({
* source: 'http://assets.paperjs.org/images/marilyn.jpg',
* position: view.center
* });
*
* raster.scale(0.5);
* raster.rotate(10);
*/
initialize: function Raster(source, position) {

@lazerwalker
Copy link
Author

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.

* 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
Copy link
Member

@lehni lehni Jun 14, 2020

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}
Copy link
Member

@lehni lehni Jun 14, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants