Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
First pass at fully documenting drupal.js
Added one line summaries, checked the 80 char limit, documented all returns and params. Made a couple of parameters types more specific.
(Postponed because this one is to be applied on top of #2493677: JSDoc for misc/ files, just getting the ball rolling)
Comment | File | Size | Author |
---|---|---|---|
#11 | core-jsdoc-drupaljs-2501913-11.patch | 14.29 KB | nod_ |
#10 | interdiff.txt | 11.19 KB | nod_ |
#10 | core-jsdoc-drupaljs-2501913-10.patch | 14.61 KB | nod_ |
Comments
Comment #1
jhodgdonwoot! Got the last of the Big Patches in. ;) I'm assuming this patch isn't done so setting to Needs Work.
Comment #2
nod_Renamed JS to JavaScript
Comment #3
chananapeeyush CreditAttribution: chananapeeyush as a volunteer and commentedissue status needs change to get review from others ?
Comment #4
jhodgdonThat depends on if nod_ is done with the patch or not?
Comment #5
chananapeeyush CreditAttribution: chananapeeyush as a volunteer and commented@jhodgdon,
Fine Thanks!.So setting the status back to needs work again.
Comment #6
nod_I'm pretty happy with the patch but my standards are probably lower than core's. Let's see what's wrong :)
Comment #7
jhodgdonThis 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.
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.
What do the backticks `` do in JSDoc? Oh I bet it's a code formatting thing. OK. Ignore this one. ;)
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.
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?
Again, these are strings, shouldn't they be '' not `` quotes?
here too
In PHP land, we standardized on TRUE and FALSE for the Boolean literals. Maybe true/false are more standard in JavaScript though?
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:
See https://www.drupal.org/node/1354#lists
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". ;)
So I would actually leave this line as it was previously.
What's a "full Drupal URL"? I think it's just a full URL that is returned, not a "drupal" URL?
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.
This doc block didn't get a one-line description added.
Attach -> Attaches [PHP standards anyway, we use verbs ending in S]
This seems like excessive indentation.
Should start with verb, like "Tests the document width for ...".
Comment #8
nod_2, 5, 6: The descriptions text are using markdown actually. So the `` are used to make monospace text and it ends up looking better in the docs when those 'code related' strings are in monospace. I don't mind either way so if you prefer '', let's go with that.
7: JS is case sensitive so TRUE and FALSE are not javascript constants. We can't use the PHP standard for that.
I'll get the rest fixed. Thanks!
Comment #9
jhodgdonEither 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:
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. ;)
Comment #10
nod_Think I got everything, Strings with '' do look better, went with that.
(and no more eslint warnings on drupal.js file after the patch :)
Comment #11
nod_Reroll since few things changed in drupal.js, added doc for the 2 new functions introduced yesterday.
Comment #12
nod_We can always refine later.
Comment #13
webchickCommitted and pushed to 8.0.x. Thanks!