See parent issue for details.

CommentFileSizeAuthor
#13 interdiff.txt1.28 KBnod_
#13 core-jsdoc-comment-2505905-13.patch5.2 KBnod_
#12 interdiff-2505905-8-12.txt977 bytesnidaismailshah
#12 jsdoc_comment_module-2505905-12.patch4.91 KBnidaismailshah
#8 jsdoc_comment_module-2505905-8.patch4.91 KBmikebell_
#4 jsdoc_comment_module-2505905-4.patch4.73 KBeiriksm
#4 interdiff2505905-1-4.txt1.16 KBeiriksm
#1 jsdoc_comment_module-2505905-1.patch4.55 KBeiriksm

Comments

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new jsdoc_comment_module-2505905-1.patch4.55 KB

Some of the scoped functions probably does not have the best docs, but they are "private" and will not end up on the docs page anyway. But of course, suggestions welcome :)

eiriksm’s picture

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

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

In several places you have "placeholder" instead of "$placeholder" in for param.

Also have some eslint warnings left: 

core/modules/comment/js/node-new-comments-link.js
  56:2  warning  Missing JSDoc @returns for function                        valid-jsdoc
  56:2  warning  Expected JSDoc for '$placeholder' but found 'placeholder'  valid-jsdoc
  72:2  warning  Missing JSDoc for parameter '$placeholder'                 valid-jsdoc
  82:2  warning  Missing JSDoc @returns for function                        valid-jsdoc
  82:2  warning  Missing JSDoc for parameter '$placeholder'                 valid-jsdoc

✖ 5 problems (0 errors, 5 warnings)
eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new interdiff2505905-1-4.txt1.16 KB
new jsdoc_comment_module-2505905-4.patch4.73 KB

That's strange. I could have sworn I ran eslint before I uploaded the patch.

Oh well, sorry about that. Here is a new one. Now I double checked that I ran it.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Reviewed & tested by the community

All good, thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Thanks!

Setting back to Needs Review... see #2504713-4: JSDoc book module

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Also... We normally want function first-line docs to start with a 3rd person verb, like "Processes" not "Process". So some of the doc blocks in node-new-comments-link.js need some attention. Thanks!

mikebell_’s picture

mikebell_ commented
StatusFileSize
new jsdoc_comment_module-2505905-8.patch4.91 KB

Updated the doc blocks as suggested by jhodgdon

emma.maria’s picture

emma.maria
she/her
commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Please, when uploading a new patch, make an interdiff file so we can see easily what has changed. Thanks!

So we should probably wait to finalize this until the question of "how to document the attach/detach behavior functions" is resolved; see [#10027109-4] and the following several comments for that.

Anyway... I reviewed the latest patch (thanks for that!) and there are still a few things to fix besides the attach docs:

  1. +++ b/core/modules/comment/js/node-new-comments-link.js
@@ -76,6 +101,12 @@
+   * Processes new comment links and add appropriate text in relevant cases.

    add => adds [to agree in verb person with rest of sentence] 

  2. +++ b/core/modules/comment/js/node-new-comments-link.js
@@ -76,6 +101,12 @@
+   *   The placeholders elements of the current page.

    placeholders => placeholder 

  3. +++ b/core/modules/comment/js/node-new-comments-link.js
@@ -105,8 +136,15 @@
+     * Either use the data embedded in the page or perform an AJAX request to
+     * retrieve the same data.
+     *
+     * @param {object} results
+     *   The server response for new comment links
+     */
     function render(results) {
       for (var nodeID in results) {
         if (results.hasOwnProperty(nodeID) && $placeholdersToUpdate.hasOwnProperty(nodeID)) {

    This doesn't make sense to me. The docs say it can use data embedded in the page, but then in the @param it looks like it's expecting a server response as input no matter what. Something must be incorrect here?

    Maybe the word "embedded" is what is making me confused? What does this mean? 

  4. +++ b/core/modules/comment/js/node-new-comments-link.js
@@ -105,8 +136,15 @@
+     *   The server response for new comment links

    Needs to end in .

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work
nidaismailshah’s picture

nidaismailshah commented
Status: Needs work » Needs review
StatusFileSize
new jsdoc_comment_module-2505905-12.patch4.91 KB
new interdiff-2505905-8-12.txt977 bytes

fixes Issue 1,2 and 4 from #10

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-jsdoc-comment-2505905-13.patch5.2 KB
new interdiff.txt1.28 KB

Fixed 3 and updated the behavior declaration to our standards.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Looks good enough

webchick’s picture

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

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 0e05f3b on 8.0.x 
    Issue #2505905 by eiriksm, nod_, nidaismailshah, mikebell_, jhodgdon:...

Status: Fixed » Closed (fixed)

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