Update the jsdoc comments to modern syntax - Part 1 #3694

brandonocasey · 2016-10-19T14:58:08Z

Description

Add @return and description where missing from functions
Add @param and description where missing
Remove un-necessary doc comments such as @class and @method
Update examples to match current code standards
Update descriptions to make more sense across the board
Keep all doc comments in a standard format
Verify that documentation looked good after running jsdoc
document @events and @fires
Depends upon remove un-needed contructor and function overrides #3721

Requirements Checklist

Feature implemented / Bug fixed
Reviewed by Two Core Contributors

misteroneill

A number of grammar and other questions/concerns.

misteroneill · 2016-10-19T17:47:20Z

src/js/big-play-button.js

@@ -32,14 +35,19 @@ class BigPlayButton extends Button {
  /**
   * Handles click for play
   *
-   * @method handleClick
+   * @return {undefined}
+   *          n/a


I'm not convinced we should have an @return tag for functions that don't explicitly return anything (i.e. undefined). That seems like a feature of the language rather than something we need to document.

misteroneill · 2016-10-19T17:49:17Z

src/js/button.js

+   *        The class name or instance of a child to add
+   *
+   * @param {Object} [options={}]
+   *        including options to be passed to children of the child.


Did we lose "Options, " here?

misteroneill · 2016-10-19T17:57:46Z

src/js/clickable-component.js

   * @return {Element}
-   * @method createEl
+   *         the element that was created


I'm not sure this isn't implied by the fact that we have @return {Element}... maybe. I'm torn.

it may be implied but I don't think its a bad thing to document

Capitalization, though. 😄

misteroneill · 2016-10-19T17:59:34Z

src/js/clickable-component.js

@@ -110,8 +127,8 @@ class ClickableComponent extends Component {
  /**
   * Allows sub components to stack CSS class names


This doesn't make much sense. It's like it's describing why the method exists - not what it does. I think something like: "Constructs a string to be used for the component's element's className." would be preferable.

Having a why isn't necessary bad, though, it should be done in a secondary paragraph or something. The first thing should definitely be what it does.

misteroneill · 2016-10-19T18:00:44Z

src/js/clickable-component.js

-   * @return {String}
-   * @method buildCSSClass
+   * @return {string}
+   *         The current css class for this ClickableComponent


This doesn't feel particularly accurate. It returns the default className for the component's element - not the current className. I think we should avoid using the term "css class" because it's got nothing to do with CSS.

misteroneill · 2016-10-19T18:12:27Z

src/js/component.js

   */
  setTimeout(fn, timeout) {
    fn = Fn.bind(this, fn);

-    // window.setTimeout would be preferable here, but due to some bizarre issue with Sinon and/or Phantomjs, we can't.
+    // window.setTimeout would be preferable here,
+    // but due to some bizarre issue with Sinon and/or Phantomjs, we can't.


I don't think this comment is relevant anymore.

misteroneill · 2016-10-19T18:13:04Z

src/js/component.js

   */
  static getComponent(name) {
    if (Component.components_ && Component.components_[name]) {
      return Component.components_[name];
    }

+    // TODO: deprecated?


What's the question here?

do we want to warn in the log that this is deprecated?

This is probably something that should just be disallowed/removed in 6.0

ok I will add an @deprication tag to the function here

misteroneill · 2016-10-19T18:14:13Z

src/js/error-display.js

@@ -49,6 +49,11 @@ class ErrorDisplay extends ModalDialog {
  }
 }

+/**
+ * Available options for an error display


I think this should be "Default options" instead of "Available options".

misteroneill · 2016-10-19T18:15:57Z

src/js/event-target.js

 */
 import * as Events from './utils/events.js';

+/**
+ * An EventTarget class that seeks to emulate much of the builtin
+ * HTML EventTarget functionalify as well as add all kinds of ease


Should be "functionalify" and I think the sentence can end there. The rest is kind of vague.

misteroneill · 2016-10-19T18:18:32Z

src/js/event-target.js

 EventTarget.prototype.allowedEvents_ = {};

+/**
+ * This function is a cross browser wrapper around addEventListener
+ * with a much nicer syntax


I think it'd be better to describe what this does rather than its implementation, the same way the off method does.

gkatsev

Good stuff.

gkatsev · 2016-10-21T22:15:32Z

src/js/close-button.js

  constructor(player, options) {
    super(player, options);
    this.controlText(options && options.controlText || this.localize('Close'));
  }

+  /**
+   * Constructs a string to be used for the component elements


Does jsdoc not provide any functionality for inheritance? So we would only need to document these in Component and then everything would be linked?

anything that you override needs to be documented, we get anything that is not overridden, in the docs for the object. There are ways to say that this method should use the parents doc comments, but doing so causes jsdoc to link to the parent function rather than the current files function (so it seems like the current file does not implement it)

gkatsev · 2016-10-21T22:25:27Z

src/js/component.js

+   * @param {Event|Object|string} event
+   *        A string (the type) or an event object with a type attribute
+   *
+   * @param {Object} [hash]


is there an "optional" indicator?

brackets [] around the parameter indicate optional

gkatsev · 2016-10-21T22:40:46Z

src/js/component.js

+   *        'width' or 'height'
+   *
+   * @return {number|boolean}
+   8         the dimention or false if nothing was set


8 instead of *.
dimension mispelled.

This doesn't return a boolean. It should default to 0, I believe.

gkatsev · 2016-10-21T22:42:45Z

src/js/component.js

   */
  static getComponent(name) {
    if (Component.components_ && Component.components_[name]) {
      return Component.components_[name];
    }

+    // TODO: deprecated?


This is probably something that should just be disallowed/removed in 6.0

gkatsev · 2016-10-21T22:44:15Z

src/js/error-display.js

@@ -27,11 +30,10 @@ class ErrorDisplay extends ModalDialog {
  /**
   * Include the old class for backward-compatibility.
   *
-   * This can be removed in 6.0.


I wonder what this refers to specifically.

brandonocasey · 2016-10-25T14:38:59Z

@gkatsev @misteroneill ready for re-review

Starting a new review!

misteroneill · 2016-10-25T15:16:12Z

As a general comment, I think it'd be good to have some kind of conventions regarding grammar. My personal feeling is that things should generally follow English grammar. So, something like this:

* @return {Component}
*         Returns itself; method can be chained.

But that's me. If more people disagree, that's fine. Thoughts?

brandonocasey · 2016-10-25T15:19:46Z

sounds good to me

misteroneill

I started adding individual line comments, but I think the whole thing could use a grammar sweep. Unless someone disagrees, I believe documentation should follow the conventions of the language in which it's written.

misteroneill · 2016-10-25T18:49:55Z

src/js/big-play-button.js

-   * @return {String} The constructed class name
-   * @method buildCSSClass
+   * @return {string}
+   *          returns default class name ie. 'vjs-big-play-button'


Couple grammatical updates here: Returns default class name, i.e., 'vjs-big-play-button'.

misteroneill · 2016-10-25T18:50:26Z

src/js/big-play-button.js

  constructor(player, options) {
    super(player, options);
  }

  /**
-   * Allow sub components to stack CSS class names
+   * Get the default css class for the BigPlayButton


If we're leaving "css" here it should be capitalized.

misteroneill · 2016-10-25T18:52:23Z

src/js/big-play-button.js

+   *
+   * @param {Object} [options]
+   *        Object of option names and values
+   */


Both "constructor" and "the" should be capitalized in this block.

misteroneill · 2016-10-25T18:52:57Z

src/js/button.js

+   * Constructor for Button
+   *
+   * @param {Player} player
+   *        the player to attach this button to


Capitalization.

misteroneill · 2016-10-25T18:53:23Z

src/js/button.js

   *
-   * @method handleKeyPress
+   * @param {Event} event
+   *        the button event that caused this function to be called


Capitalization.

misteroneill · 2016-10-25T18:55:04Z

src/js/clickable-component.js

   * @return {Element}
-   * @method controlText
+   *         the control text element
   */


This could probably stand to have some capitalization added to both the @return and the description above.

misteroneill · 2016-10-25T18:55:26Z

src/js/clickable-component.js

+   *
+   * @return {string|ClickableComponent}
+   *         - The control text when getting
+   *         - Returns itself when setting; method can be chained.


Does this get Markdown-ified?

misteroneill · 2016-10-25T18:56:06Z

src/js/clickable-component.js

-   * @return {String}
-   * @method buildCSSClass
+   * @return {string}
+   *         the default className for a component


Capitalization and sentences should end in periods.

misteroneill · 2016-10-25T18:57:10Z

src/js/clickable-component.js

-   * @method handleKeyPress
+   * @param {Event} event
+   *        the event that caused this to be called, in most cases
+   *        this will be the `keydown` event


I think we should leave out everything after the comma. And capitalize.

misteroneill · 2016-10-25T19:02:35Z

src/js/component.js

+ *   var button = player.addChild('button');
+ *
+ *   console.log(button.el());
+ *   // will have the same html result as the previous example


I'm wondering if we should really have code examples in API docs or leave that to the manual documentation/guides.

A part of me feels like we need to have doc comments that link to guides, and the guides should be the ones with the examples. Maybe another PR to rip examples out into manual documentation at a later date?

Sounds good.

There is an upside to have examples here:

more likely to get updated if things change

A user doesn't have to go to a guide to read up on how to use it

But I'm ok with moving it to a guide and linking it here in a separate PR pass

Another benefit of having the jsdoc comments not contain examples. Is that we can lint markdown examples with eslint. Which cannot be done with jsdoc markdown examples.

gkatsev · 2016-11-02T19:51:43Z

src/js/clickable-component.js

+   * This gets called when a `ClickableComponent` gets:
+   * - Clicked (via the `click` event, listening starts in the constructor)
+   * - Tapped (via the `tap` event, listening starts in the constructor)
+   * - Gains focus (via the `focus` event). Causes `ClickableComponent`


This doesn't listen to keydown.

When focus is triggered handleFocus is called.

gkatsev · 2016-11-02T19:53:37Z

src/js/close-button.js

+   * This gets called when a `CloseButton` gets:
+   * - Clicked (via the `click` event, listening starts in the constructor)
+   * - Tapped (via the `tap` event, listening starts in the constructor)
+   * - Gains focus (via the `focus` event). Causes `ClickableComponent`


same as above.

gkatsev · 2016-11-02T19:58:47Z

src/js/component.js

+ *   var button = player.addChild('button');
+ *
+ *   console.log(button.el());
+ *   // will have the same html result as the previous example


There is an upside to have examples here:

more likely to get updated if things change

A user doesn't have to go to a guide to read up on how to use it

But I'm ok with moving it to a guide and linking it here in a separate PR pass

gkatsev · 2016-11-02T20:01:17Z

src/js/component.js

-   * Whenever a property is an object on both options objects
-   * the two properties will be merged using mergeOptions.
+   * Deep merge of options objects with new options.
+   * > Note: When a property of is an object on both `obj` and `options`.


"When a property of is an object" is weird.
Maybe something like:
When both obj and options contain a property whose value is an object, the two properties get merged using mergeOptions

gkatsev · 2016-11-02T20:04:50Z

src/js/component.js

+   * - `otherComponent.on('eventName', myFunc)`
+   * Is that the listeners will get cleaned up when either component gets disposed.
+   * It will also bind `myComponent` as the context of `myFunc`.
+   * > **NOTE**: If you remove the element from the DOM that has used `on` you need to


I think usage of NOTE is consistent.

gkatsev · 2016-11-02T20:26:11Z

src/js/component.js

@@ -1378,12 +1711,16 @@ class Component {
  }

  /**
-   * Registers a component
+   * Register a `Component` with `videojs` given the name and the component.


Should we add a note that Techs shouldn't be registered as Components?

gkatsev · 2016-11-02T20:28:20Z

src/js/event-target.js

+ *
+ * @typedef {Object} EventTarget~Event
+ * @see [Properties]{@link https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent}
+ */


new line, please

gkatsev · 2016-11-02T20:28:36Z

src/js/event-target.js

+ *
+ * @param {Object} [hash]
+ *        hash of data sent during the event
+ */


new line, please

gkatsev · 2016-11-02T20:29:27Z

src/js/event-target.js

+ */
+/**
+ * An object containing event names as keys and true/false values as values. If an
+ * event name is set to a true value here {@link EventTarget#trigger} will have extra.


This sentence is incomplete. What will trigger have extra of?

gkatsev · 2016-11-02T20:31:43Z

src/js/event-target.js

+ *
+ * If the name of the Event that is being triggered is in `EventTarget.allowedEvents_`
+ * then trigger will also call the `on` + `uppercaseEventName` function. Example:
+ * the 'click' is in in `EventTarget.allowedEvents_` trigger will attempt to call


click is in allowedEvents_, so, trigger will attempt to call onclick if it exists.

@see

added docs where missing updated examples where broken or where they did not make sense eslinted with eslint-plugin-jsdoc change @link to @see better @return descriptions for chainable methods ran hemmingway over the jsdoc comments custom event type custom callback types better @Listens, @fires, and @event documentation

made jsdoc recursive

misteroneill requested changes Oct 19, 2016

View reviewed changes

brandonocasey changed the title ~~updated all docs to follow modern jsdoc 3 standards~~ Update the jsdoc comments to modern syntax - Part 1 Oct 20, 2016

brandonocasey mentioned this pull request Oct 20, 2016

Update the jsdoc comments to modern syntax - Part 2 #3698

Merged

2 tasks

gkatsev requested changes Oct 21, 2016

View reviewed changes

brandonocasey added the documentation label Oct 24, 2016

misteroneill previously approved these changes Oct 25, 2016

View reviewed changes

misteroneill requested changes Oct 25, 2016

View reviewed changes

gkatsev approved these changes Oct 26, 2016

View reviewed changes

misteroneill approved these changes Oct 31, 2016

View reviewed changes

brandonocasey added the confirmed label Oct 31, 2016

gkatsev requested changes Nov 2, 2016

View reviewed changes

brandonocasey added 3 commits November 7, 2016 10:54

updated event definitions

1ac1f02

made jsdoc recursive

code review changes

8e44512

gkatsev modified the milestone: 5.14 Nov 15, 2016

gkatsev approved these changes Dec 2, 2016

View reviewed changes

gkatsev merged commit 1a0b281 into videojs:master Dec 2, 2016

brandonocasey deleted the chore/modernize-docs-1 branch December 6, 2016 16:20

		@@ -110,8 +127,8 @@ class ClickableComponent extends Component {
		/**
		* Allows sub components to stack CSS class names

Update the jsdoc comments to modern syntax - Part 1 #3694

Update the jsdoc comments to modern syntax - Part 1 #3694

Conversation

brandonocasey commented Oct 19, 2016 • edited Loading

Description

Requirements Checklist

misteroneill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Oct 25, 2016

misteroneill commented Oct 25, 2016

brandonocasey commented Oct 25, 2016

misteroneill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Oct 19, 2016 •

edited

Loading

brandonocasey Oct 24, 2016 •

edited

Loading