For more info, see parent issue.

CommentFileSizeAuthor
#19 core-jsdoc-ckeditor-19.patch29.53 KBnod_
#19 interdiff.txt1.12 KBnod_
#17 interdiff.txt571 bytesnod_
#17 core-jsdoc-ckeditor-17.patch28.85 KBnod_
#1 jsdoc_ckeditor_module-2505363-1.patch26.63 KBeiriksm
#4 jsdoc_ckeditor_module-2505363-4.patch26.58 KBeiriksm
#4 interdiff1-4.txt2.28 KBeiriksm
#6 interdiff4-6.txt1.55 KBeiriksm
#6 jsdoc_ckeditor_module-2505363-6.patch26.68 KBeiriksm
#9 jsdoc_ckeditor_module-2505363-9.patch26.76 KBeiriksm
#9 interdiff2505363-6-9.txt1.73 KBeiriksm
#11 jsdoc_ckeditor_module-2505363-11.patch26.75 KBjoshi.rohit100
#11 interdiff-11.txt753 bytesjoshi.rohit100
#14 jsdoc_ckeditor_module-2505363-13.patch28.25 KBmlevasseur
#14 interdiff-2505363-11-13.txt2.14 KBmlevasseur
#16 interdiff-2505363-14-16.txt1.07 KBmlevasseur
#16 jsdoc_ckeditor_module-2505363-16.patch28.81 KBmlevasseur
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
FileSize
jsdoc_ckeditor_module-2505363-1.patch26.63 KB

Creating patches for these types of issues just became a lot easier, since #2494177: Enable ESLint warning for missing JSDoc got committed. :)

Granted, this might oversee some issues, but still makes it easier to do the easy ones. One example is that eslint does not catch comment lines over 80 characters (although I tried to go through the whole module for that as well in this patch).

Some of the parameters are not the best descriptions, but please feel free to make suggestions in that direction.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
Status: Active » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Needs work

Woot! thanks for taking on a hard one :) 

  1. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
@@ -93,20 +94,22 @@
+     * Translates a change in CKEditor config DOM structure into the config
+     *   model.

@@ -127,10 +130,12 @@
+     * Translates a change in CKEditor config DOM structure into the config
+     *   model.

@@ -155,15 +160,16 @@
+     * Opens a Drupal dialog with a form for changing the title of a button
+     *   group.

    Should be rephrased so that it fits on one line. 

  2. +++ b/core/modules/ckeditor/js/ckeditor.js
@@ -83,13 +94,19 @@
+     *   The result of the attachInlineEditor function.

    a @link could be useful here. 

  3. +++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
@@ -210,8 +210,11 @@
+   *   The ckeditor editor object

    /ckeditor/CKEditor/ 

  4. +++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
@@ -210,8 +210,11 @@
+   * @return {?HTMLElement}
+   *   The selected link element, or null.
    *
-   * @return {?bool}

    Agreed, thanks for correcting that one, was sloppy from me.

    http://docs.ckeditor.com/#!/api/CKEDITOR.dom.elementPath 

  5. +++ b/core/modules/ckeditor/js/views/KeyboardView.js
@@ -1,6 +1,7 @@
+ * A Backbone View that provides the aural view of CKEditor keyboard UX
+ *   configuration.

    Should be one line

Patch fixed the 42 eslint jsdoc warnings.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
Status: Needs work » Needs review
FileSize
jsdoc_ckeditor_module-2505363-4.patch26.58 KB
interdiff1-4.txt2.28 KB

Woot! thanks for taking on a hard one :)

:) I just started doing one every night I get the time, and I do them alphabetically. Fun fact: seems like ckeditor and views_ui make up about 25% of the eslint jsdoc errors in the modules directory :)

1. Fixed.
2. Hm. Not sure what you mean. @link to attachInlineEditor? It's the actual function i am documenting. Seems unnecessary. Could link to CKEDITOR.inline, but we don't have that in the docs.
3-5: Fixed.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Needs work

Right, the return description could use so work then. Because it doesn't tell me anything too useful as is. What about something like "Result of CKEditor.inline() call." or something.

Beside that, it looks good to me :)

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
Status: Needs work » Needs review
FileSize
interdiff4-6.txt1.55 KB
jsdoc_ckeditor_module-2505363-6.patch26.68 KB

Right, the return description could use so work then. Because it doesn't tell me anything too useful as is.

Hard to argue with that :) I totally agree.

I looked over some of the other @return in there, and they were composed the same way. In addition to that, the function we are discussing does not return the return value of CKEditor.inline(). It returns !!CKEditor.inline(). Which means it returns true if the CKEditor.inline() returns something truthy. It usually returns an editor instance. Same thing goes for another function in that file, so I also updated that one.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Needs work

looking at the interdiff: I would keep 'true' lowercase, js being case sensitive and all. we can put `` on it. Or maybe turn it like "Whether the call to `CKEDITOR.replace()` created an editor." that way we sidestep the issue. Not sure what's best would need a second opinion.

The rest looks good.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented

I see your point.

As you, I am not sure what the best way to word this would be. I am not sure "Whether the call to `CKEDITOR.replace()` created an editor." is the best alternative either. If someone wants to suggest something here, that would be great.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
Status: Needs work » Needs review
FileSize
jsdoc_ckeditor_module-2505363-9.patch26.76 KB
interdiff2505363-6-9.txt1.73 KB

OK, so went with a variation of the above suggestion.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Needs work

The behaviors are missing some docs, either missing a one line summary or the @prop attach thing.

  1. +++ b/core/modules/ckeditor/js/views/AuralView.js
@@ -1,6 +1,7 @@
+ *   configuration.

    Strange indenting, also longer than 80 char.

    Losing the "A " at the begining should make the line fit. 

  2. +++ b/core/modules/ckeditor/js/views/KeyboardView.js
@@ -1,6 +1,7 @@
+ *

    extra space

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented
Status: Needs work » Needs review
FileSize
jsdoc_ckeditor_module-2505363-11.patch26.75 KB
interdiff-11.txt753 bytes
eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
Status: Needs review » Needs work

Still missing the behavior docs (as pointed out above).

See #2504713-7: JSDoc book module for context.

Feel free to add it, or if you don't have the time, I'll add it in a few days.

mlevasseur queued 11: jsdoc_ckeditor_module-2505363-11.patch for re-testing.

mlevasseur’s picture

mlevasseur CreditAttribution: mlevasseur at Acro Commerce commented
Status: Needs work » Needs review
FileSize
jsdoc_ckeditor_module-2505363-13.patch28.25 KB
interdiff-2505363-11-13.txt2.14 KB

I'm not sure if I documented all the behaviours; I could only find the two files (ckeditor.admin and ckeditor.stylescombo.admin) that had un-documented behaviours. Hopefully I did what you were wanting?

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
Status: Needs review » Needs work

Nice!

Just 2 more things:

+++ b/core/modules/ckeditor/js/ckeditor.admin.js
@@ -10,7 +10,14 @@
+   * Sets the configuration behaviour and creates configuration views for the CKEditor toolbar

This line is over 80 characters

Also, there is a drupal behavior in ckeditor/js/ckeditor.drupalimage.admin.js

mlevasseur’s picture

mlevasseur CreditAttribution: mlevasseur at Acro Commerce commented
Status: Needs work » Needs review
FileSize
jsdoc_ckeditor_module-2505363-16.patch28.81 KB
interdiff-2505363-14-16.txt1.07 KB

Updating with changes.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community
FileSize
core-jsdoc-ckeditor-17.patch28.85 KB
interdiff.txt571 bytes

Small fix for #16, rest still looks good.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Reviewed & tested by the community » Needs work
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.12 KB
core-jsdoc-ckeditor-19.patch29.53 KB

New parameter introduced in #2510138: Unidirectional editor configuration -> filter settings syncing breaks when button name does not equal the command name documented. interdiff with #16

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 9501ed4 on 8.0.x 
    Issue #2505363 by eiriksm, nod_, mlevasseur, joshi.rohit100: JSDoc...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.