See parent issue for details.

CommentFileSizeAuthor
#15 jsdoc_color_module-2505721-15.patch2.77 KBeiriksm
#13 jsdoc_color_module-2505721-13.patch2.85 KBeiriksm
#8 interdiff-2505721-6-8.txt388 byteseiriksm
#8 jsdoc_color_module-2505721-8.patch2.6 KBeiriksm
#5 interdiff1-5.txt2.06 KBeiriksm
#5 jsdoc_color_module-2505721-5.patch2.57 KBeiriksm
#1 jsdoc_color_module-2505721-1.patch2.23 KBeiriksm
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
Status: Active » Needs review
FileSize
jsdoc_color_module-2505721-1.patch2.23 KB
nod_’s picture

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

Missing a description on the Drupal.color namespace too.

  1. +++ b/core/modules/color/color.js
@@ -95,10 +95,14 @@
        * @param {Array} ref1
...
        * @param {Array} ref2

    We can even make the type more specific: Array.<number>. 

  2. +++ b/core/modules/color/color.js
@@ -95,10 +95,14 @@
+       *   Reference part 1. An array of HSL codes.
...
+       *   Reference part 2. An array of HLS codes.

    HSL or HLS? :p

    maybe call them "First HSL color reference" and "Second HLS color reference"? 

  3. +++ b/core/modules/color/color.js
@@ -143,9 +147,14 @@
+       *   The input element for the color choosing.
...
+       *   The color chosen

    Those ones sounds like they could be turned better but I don't have an idea. 

  4. +++ b/core/modules/color/preview.js
@@ -14,11 +15,17 @@
     /**
      * @param {Element} context

    Missing a summary. 

  5. +++ b/core/modules/color/preview.js
@@ -14,11 +15,17 @@
+     *   Height, in pixels.
...
+     *   Width, in pixels.

    Of height/width of what?

Thanks again, feels good to be reviewing doc patches :D

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented

Patch fixes the 15 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 » Postponed

Postponing, waiting for #2505741: Remove unused Drupal.color.callback, as the comments above is related to that function and we want to remove it.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
FileSize
jsdoc_color_module-2505721-5.patch2.57 KB
interdiff1-5.txt2.06 KB
eiriksm’s picture

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

Not postponed anymore, was a bit quick there with my conclusions :)

nod_’s picture

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

For the behavior description I'd go a bit more explicit: "Displays farbtastic color selector and initialize color administration UI." (not great but a little better).

Beside that, all good.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
FileSize
jsdoc_color_module-2505721-8.patch2.6 KB
interdiff-2505721-6-8.txt388 bytes

Sounds better, agreed :)

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
nod_’s picture

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

Fixes the 15 eslint jsdoc warnings. Happy with it.

jhodgdon’s picture

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

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

Actually.. also Drupal.color still needs to have docs added to its doc block (at least a one-line description).

The rest looks good, thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work
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_color_module-2505721-13.patch2.85 KB

Added behavior doc and one small line about Drupal.color

nod_’s picture

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

Hate to NW for that but @namespace should be after the one line description. There is only the @file tag that is before any description.

( edit ) and there is an extra line before the @file docblock in preview.js, that needs to be removed.

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_color_module-2505721-15.patch2.77 KB

Ah, sorry. That was a bit sloppy.

And that new line. Wonder where that came from.

Changed the 2 things above. No interdiff since the patch is still so small.

nod_’s picture

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

Nice, thanks!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Confirmed that this resolves all the current eslint errors for the module:

[mandelbrot:maintainer | Sat 16:45:36] $ eslint ./core/modules/color
[mandelbrot:maintainer | Sat 16:46:00] $

This issue only changes documentation, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x. Thanks!

  • xjm committed b73d887 on 8.0.x 
    Issue #2505721 by eiriksm, nod_, jhodgdon: JSDoc color module

Status: Fixed » Closed (fixed)

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