JSDoc drupal.js

woot! Got the last of the Big Patches in. ;) I'm assuming this patch isn't done so setting to Needs Work.

issue status needs change to get review from others ?

That depends on if nod_ is done with the patch or not?

@jhodgdon,
Fine Thanks!.So setting the status back to needs work again.

This looks pretty good to me, thanks!

I have a few suggestions for how it could be improved... well, there are more than a few, but they're pretty minor.

1. +++ b/core/misc/drupal.js
   @@ -1,19 +1,25 @@
    /**
     * A jQuery object.
     *
   + * HTMLElement or collection of HTMLElement held by a jQuery object.
   + *
     * @typedef {object} jQuery
     *
     * @prop {number} length=0
   + *   Number of HTMLElement contained in the jQuery object.
     */
    
   

   This doc block confused me until nod_ explained it in IRC. So maybe it could be a bit clearer, as the point of docs is to clarify? :) Well also in this case the point is to make an entry on the JSDocs site... But anyway.

   My suggestion would be something like this:

   first line:

   A jQuery object, typically the return value from a $() call.

   next section:

   Holds an HTMLElement or a collection of HTMLElements.

   And then for the docs in the number property, I think it should be plural "number of HTMLElement elements" or just "number of elements" (since we've already said it's holding a particular class of things), because "number of element" doesn't seem like very good English.

2. +++ b/core/misc/drupal.js
   @@ -1,19 +1,25 @@
   + * contained in `drupalSettings` is used during behavior initialization.
   

   What do the backticks `` do in JSDoc? Oh I bet it's a code formatting thing. OK. Ignore this one. ;)

3. +++ b/core/misc/drupal.js
   @@ -22,6 +28,9 @@
   + * Content of this variable is automatically created by Drupal when using locale
   + * module. It holds the translation of strings used on the page.
   + *
   

   Needs "the" at the beginning and before "locale module".

   Also... normally when referring to a module in PHP docs we would either say machinename.module or use the human-readable name. So this would either be "locale.module" or "the Interface Translation module", not "locale module".

   And one more thing... does this only have the strings that are needed on the current page, or does it have all the translations for all strings? The docs seem to say it's just on the current page.

4. +++ b/core/misc/drupal.js
   @@ -123,10 +135,12 @@ if (window.jQuery) {
   +   *   One of `unload`, `move` or `serialize`.
   

   In the Drupal project as a whole, we have a standard of using serial commas. So this should be:

   unload, move, or serialize
   [add comma before the "or"]

   Also as these are strings, shouldn't they be using regular '' quotes not backticks?

5. +++ b/core/misc/drupal.js
   @@ -230,17 +243,16 @@ if (window.jQuery) {
   +   *   - `unload`: The context element is being removed from the DOM.
   +   *   - `move`: The element is about to be moved within the DOM (for example,
       *     during a tabledrag row swap). After the move is completed,
   

   Again, these are strings, shouldn't they be '' not `` quotes?

6. +++ b/core/misc/drupal.js
   @@ -248,7 +260,7 @@ if (window.jQuery) {
   +   *   - `serialize`: When an Ajax form is submitted, this is called with the
   

   here too

7. +++ b/core/misc/drupal.js
   @@ -288,8 +300,11 @@ if (window.jQuery) {
   +   *   Returns true if the document's `clientWidth` is bigger than `width`,
   +   *   returns false otherwise.
       *
   

   In PHP land, we standardized on TRUE and FALSE for the Boolean literals. Maybe true/false are more standard in JavaScript though?

8. +++ b/core/misc/drupal.js
   @@ -327,13 +342,14 @@ if (window.jQuery) {
   +   *    - `!variable`: inserted as is
   +   *    - `@variable`: escape plain text to HTML ({@link Drupal.checkPlain})
   +   *    - `%variable`: escape text and theme as a placeholder for user-submitted
       *      content ({@link Drupal.checkPlain} +
       *      {@link Drupal.theme}('placeholder'))
       *
   

   In PHP docs, all list items should end in . -- probably should do the same for JS? Also we'd normally after a : start with a capital letter. And there's the backtick thing... So the first one would be, I think:

   +   *    - '!variable': Inserted as is.
   

   See https://www.drupal.org/node/1354#lists

9. +++ b/core/misc/drupal.js
   @@ -327,13 +342,14 @@ if (window.jQuery) {
       * @return {string}
   +   *   Returns the formatted string.
       *
   

   In PHP docs, we don't normally start the @return docs with "Returns the", we just leave this out. Same as in @param we wouldn't start with "Pass in the". ;)

10. +++ b/core/misc/drupal.js
    @@ -429,11 +445,12 @@ if (window.jQuery) {
    -   *   The translated string.
    +   *   Returns the translated string.
        */
    

    So I would actually leave this line as it was previously.

11. +++ b/core/misc/drupal.js
    @@ -457,6 +474,7 @@ if (window.jQuery) {
    +   *   Returns the full Drupal URL.
    

    What's a "full Drupal URL"? I think it's just a full URL that is returned, not a "drupal" URL?

12. OK, that's it for what is in the patch. Then I looked through the patched durpal.js file and found a couple of other things that still look like they need attention.

      /**
       * Callback function initializing code run on page load and Ajax requests.
       *
       * @callback Drupal~behaviorAttach
    

    This doc block is out in the middle of nowhere so I'm not sure about it, but it looks like it's documenting a function. So normally in PHP docs land we'd want to start this with a verb, so something like:

    Initializes the JavaScript behaviors for page loads and Ajax requests.

13. Same for the detach callback that comes next.
14. Just after that:
    

      /**
       * @typedef {object} Drupal~behavior
    

    This doc block didn't get a one-line description added.

15.   /**
       * Attach all registered behaviors to a page element.
     ...
       */
      Drupal.attachBehaviors = function (context, settings) {
    

    Attach -> Attaches [PHP standards anyway, we use verbs ending in S]

16. The detach doc block below should also start with Detaches. And the checkPlain one should be Encodes, formatString -> Replaces, etc.
17. And in the middle of the Attach doc block:
    

       * Behaviors should use
       *     `var elements = $(context).find(selector).once('behavior-name');`
       * to ensure the behavior is attached only once to a given element. (Doing so
    

    This seems like excessive indentation.

18.   /**
       * Helper to test document width for mobile configurations.
    ...
       */
      Drupal.checkWidthBreakpoint = function (width) {
    

    Should start with verb, like "Tests the document width for ...".

Either way then... Well I guess I think that anything that is a string, when mentioned in docs, should usually be put in '' to differentiate it from a variable, class, constant, etc. Especially in JS, which doesn't have a $ prefix for variable names. So if you want things to be monospace that are strings, you could do `'thestring'` right? A bit awkward in the source but it should look good on the docs site. Anyway, I don't feel extremely strongly about this, and particularly in PHP docs, we do sometimes leave out the '' on strings. Example:

* blah blah blah ... an array with the following elements:
* - element_1: Something here. Notice the element name is not in '' quotes if it's before : in a list.

In fact we have that suggestion about keys for arrays in
https://www.drupal.org/node/1354#lists
but otherwise we would normally want strings in quotes in docs. In lists, on api.drupal.org, everything before : in a list item gets formatted in bold.

Anyway... Either way. ;)

Committed and pushed to 8.0.x. Thanks!