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.
For more info, see parent issue.
Comment | File | Size | Author |
---|---|---|---|
#19 | core-jsdoc-ckeditor-19.patch | 29.53 KB | nod_ |
#19 | interdiff.txt | 1.12 KB | nod_ |
#16 | interdiff-2505363-14-16.txt | 1.07 KB | mlevasseur |
#16 | jsdoc_ckeditor_module-2505363-16.patch | 28.81 KB | mlevasseur |
Comments
Comment #1
eiriksmCreating 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.
Comment #2
eiriksmComment #3
nod_Woot! thanks for taking on a hard one :)
Should be rephrased so that it fits on one line.
a @link could be useful here.
/ckeditor/CKEditor/
Agreed, thanks for correcting that one, was sloppy from me.
http://docs.ckeditor.com/#!/api/CKEDITOR.dom.elementPath
Should be one line
Patch fixed the 42 eslint jsdoc warnings.
Comment #4
eiriksm:) 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.
Comment #5
nod_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 :)
Comment #6
eiriksmHard 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 theCKEditor.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.Comment #7
nod_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.
Comment #8
eiriksmI 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.
Comment #9
eiriksmOK, so went with a variation of the above suggestion.
Comment #10
nod_The behaviors are missing some docs, either missing a one line summary or the @prop attach thing.
Strange indenting, also longer than 80 char.
Losing the "A " at the begining should make the line fit.
extra space
Comment #11
joshi.rohit100Comment #12
eiriksmStill 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.
Comment #14
mlevasseur CreditAttribution: mlevasseur at Acro Commerce commentedI'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?
Comment #15
eiriksmNice!
Just 2 more things:
This line is over 80 characters
Also, there is a drupal behavior in ckeditor/js/ckeditor.drupalimage.admin.js
Comment #16
mlevasseur CreditAttribution: mlevasseur at Acro Commerce commentedUpdating with changes.
Comment #17
nod_Small fix for #16, rest still looks good.
Comment #18
nod_Comment #19
nod_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
Comment #20
nod_Comment #21
webchickCommitted and pushed to 8.0.x. Thanks!