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

Update the jsdoc comments to modern syntax - Part 3 #3708

Merged
merged 4 commits into from
Dec 2, 2016
Merged

Update the jsdoc comments to modern syntax - Part 3 #3708

merged 4 commits into from
Dec 2, 2016

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Oct 24, 2016

Description

  • player.js
  • setup.js
  • slider.js

@brandonocasey brandonocasey changed the title Update the jsdoc comments to modern syntax - Part 2 Update the jsdoc comments to modern syntax - Part 3 Oct 24, 2016
* @param {Function=} ready Ready callback function
* @class Player
* After an instance has been created it can be accessed globally again
* by calling `videojs('example_video_1');`
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we ought to mention that you can do videojs.players.example_video_1 too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure I will add that in

* @param {Object} [options]
* Object of option names and values.
*
* @param {Component~ReadyCallback} [ready]
Copy link
Member

Choose a reason for hiding this comment

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

What is this Component~ReadyCallback type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its a custom typedef that is defined in the part 1 PR.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome.

@@ -356,9 +483,10 @@ class Player extends Component {
}

/**
* Create the component's DOM element
* Create the `Player`s DOM element.
Copy link
Member

Choose a reason for hiding this comment

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

Should have an apostrophe.

Copy link
Member

Choose a reason for hiding this comment

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

There are a few places where there is a missing apostrophe below as well.

/**
* @event Player#loadstart
* @type {EventTarget~Event}
*/
Copy link
Member

Choose a reason for hiding this comment

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

This is documented above - does it need to be documented again?

Copy link
Member

Choose a reason for hiding this comment

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

Same is true of firstplay in a few places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will see if there is any way to have something like a "main definition" that these events can point to. I think it is still important to be able to see where events are used in the code from the docs.

@brandonocasey brandonocasey added the needs: LGTM Needs one or more additional approvals label Nov 9, 2016
@misteroneill misteroneill added confirmed and removed needs: LGTM Needs one or more additional approvals labels Nov 10, 2016
@gkatsev gkatsev modified the milestone: 5.14 Nov 15, 2016
@gkatsev gkatsev merged commit eb2093e into videojs:master Dec 2, 2016
@brandonocasey brandonocasey deleted the chore/modernize-docs-3 branch December 6, 2016 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants