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] fix inline docs of windowResized and other event handler functions etc. #4988

Closed
1 task done
fal-works opened this issue Jan 10, 2021 · 8 comments · Fixed by #4989
Closed
1 task done

[docs] fix inline docs of windowResized and other event handler functions etc. #4988

fal-works opened this issue Jan 10, 2021 · 8 comments · Fixed by #4989

Comments

@fal-works
Copy link
Contributor

fal-works commented Jan 10, 2021

Most appropriate sub-area of p5.js?

  • Accessibility (Web Accessibility)

My main intension is to supplement the machine-readable documentation that some other projects (such as p5.ts) rely on.


I've found some points to be fixed in the inline documentation.
Will be happy to create a pull request (not very familiar with it though; will read the contributor docs).

Description and arguments of windowResized()

According to the actual code, the two below can be added:

  • Explanation about return false,
    same as other event handlers such as mousePressed().
    Actually this can be any falsy value (while only false is valid in other event handlers), but maybe it's better to write it consistently with other event handler functions.
  • @param {Object} [event],
    which is an UIEvent.
    (ADD: same for keyboard event handlers such as keyPressed(), which lack @param tag even though they accept KeyboardEvent argument)

Return type of mousePressed() and other event handlers

mousePressed() and other similar functions may return either void or boolean.
So I'd like to add something like:

@return  {*} (optional) `false` to prevent default behavior.

Not sure if the type {*} is correct (it's also used in random() with an Array argument);
In TypeScript it would be any or void | boolean, and I suppose the latter cannot be described in YUIDoc.

Arguments of int() (conversion function)

Not really related to the above, but
@param {Integer} [radix] is missing for the case if the first argument is an Array.

@welcome
Copy link

welcome bot commented Jan 10, 2021

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

@fal-works fal-works changed the title [docs [docs] fix inline docs of windowResized and other event handler functions etc. Jan 10, 2021
@limzykenneth
Copy link
Member

@param {Object} [event],
which is an UIEvent.
(ADD: same for keyboard event handlers such as keyPressed(), which lack @param tag even though they accept KeyboardEvent argument)

I don't quite understand this, windowResized() does not accept any arguments (at least not expecting the user's definition of it to include one) so not sure how this applies.

mousePressed() and other similar functions may return either void or boolean.
So I'd like to add something like:

For me that's a bit too jargon-y especially using void. The current description about optionally adding return false is clearer in my opinion, if the return signature can do something that can be understood intuitively then we can go for it.

@fal-works
Copy link
Contributor Author

Thanks for the comment, just changed the PR to draft.
Will write a little more soon.

@fal-works
Copy link
Contributor Author

So, sorry for confusing.

First, what are described in the current documentation:

tag: @param [event] description: return false
windowResized() No No
mousePressed() etc Yes Yes
keyPressed() etc No Yes
touchStarted() etc Yes Yes

And what are common to all of these functions:

  • The actual implementation allows that a user-defined function has an argument for receiving the fired event object.
  • The actual implementation calls preventDefault() on the event object, if the user-defined function returns false.
  • The documentation does not have any @return tag.

Given that, regarding what you mentioned:

About the argument of windowResized()

Can be checked like this:

// sketch.js
function setup() {}
function windowResized(event) {
  console.log(event);
}
function mousePressed(event) {
  console.log(event);
}
function keyPressed(event) {
  console.log(event);
}
// to see details of event:    for (const key in event) console.log(`${key}: ${event[key]}`);

About return false

Yes, I didn't intend to expose the term void to users. And the current description is already clear enough.

What I want to do is just adding the missing YUIDoc @return tag, because the absense of this tag should mean that the method never returns a value, which doesn't match the description as well as the actual behavior.

...and some more about windowResized()

which I probably should fix in my PR.
Sorry again for bothering, but I just noticed the below:

  1. Both MDN and TypeScript claim that the resize event is of UIEvent type,
    but actually it seems to be just the super-type Event (cf).
    So maybe I should write Event instead of UIEvent in the comment.

  2. The code below might have no effect, because the resize event seems to be event.cancelable === false.

    executeDefault = context.windowResized(e);
    if (executeDefault !== undefined && !executeDefault) {
    e.preventDefault();
    }

    So now I think that maybe I should stop adding the description about return false for windowResized().
    And I'm not sure whether the current implementation should be left as is.

Please let me know if there are any more concerns.

@limzykenneth
Copy link
Member

Yes, I didn't intend to expose the term void to users. And the current description is already clear enough.
What I want to do is just adding the missing YUIDoc @return tag, because the absense of this tag should mean that the method never returns a value, which doesn't match the description as well as the actual behavior.

Wouldn't adding the @return tag also add the return field in the documentation as well?

The [event] parameter can be added for the windowResized() inline with other functions. As for return false, as you mentioned it does nothing so let's leave it for now, for the implementation itself we'll need to see if the specs themselves define the window resize event as not cancellable or it's just a browser convention. We also need to consider if changing the implementation here will cause a breaking change or not.

@fal-works
Copy link
Contributor Author

Re implementation of windowResized(), I agree, let's leave it for now.

Re @return tag,

Wouldn't adding the @return tag also add the return field in the documentation as well?

Indeed it does, and yes, I understand your concern about the risk of confusing users.
How about this?

/**
 * @return {*} false if any default behavior should be prevented for this event. (Optional)
 */

screenshot 2021-01-23

Or if * looks odd, Any instead of *?

I also thought of simply omitting the type, which is a valid YUIDoc and will also hide the Return field in the reference, however the contributor docs says "If there is no return type, do not include @return".

If it's still difficult to add @return, let's leave it as-is, but it should be discussed somewhen in future how to improve/maintain the machine-readable documentation while keeping the public reference being intuitive (also related to #4991).

@limzykenneth
Copy link
Member

Or if * looks odd, Any instead of *?

Any would be better than * in this case though I would have preferred [boolean] to match how we are marking parameters as optional and to be clear that usually only a boolean value will be returned. I can see maybe something like typescript would not like that though.

However for the most part, accessibility of the reference will always take priority over machine-readability. If YUIDoc or typescript isn't able to support something like [boolean] then we can either go with Any or keep the existing behaviour for now.

fal-works added a commit to fal-works/p5.js that referenced this issue Mar 18, 2021
remove description about "return false"
remove @param, @return tags
(see processing#4988)
fal-works added a commit to fal-works/p5.js that referenced this issue Mar 18, 2021
fal-works added a commit to fal-works/p5.js that referenced this issue Mar 18, 2021
@fal-works
Copy link
Contributor Author

Sorry for being so late! And thank you for all of the comments above.

Let's leave the return tag stuff as-is (for now) as it seems to be a little difficult to keep both human- and machine-readability in the current circumstances (even though they don't necessarily have to be exclusive; hope we can find any way to improve the documentation system in future).

Updated the PR #4989.

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 a pull request may close this issue.

2 participants