Quick overhaul of block module javascript standards and jsdoc.

CommentFileSizeAuthor
#11 core-jsdoc-block-2502629-11.patch6.44 KBnod_
#9 interdiff-2502629-6-9.txt2.12 KBeiriksm
#9 jsdoc_block_module-2502629-9.patch6.83 KBeiriksm
#6 interdiff3-6.txt1.65 KBeiriksm
#6 jsdoc_block_module-2502629-6.patch5.16 KBeiriksm
#3 interdif.txt3 KBeiriksm
#3 jsdoc_block_module-2502629-3.patch4.56 KBeiriksm
#1 jsdoc_block_module-2502629-1.patch6.61 KBeiriksm

Comments

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new jsdoc_block_module-2502629-1.patch6.61 KB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

This is not just a documentation addiction, you've also moved checkEmptyRegions around.
All documentation additions look good but I think the move of checkEmptyRegions should be in a different issue.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new jsdoc_block_module-2502629-3.patch4.56 KB
new interdif.txt3 KB

Good point.

Just very tempting to clean up things when one looks through files :)

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented

Also opened issue for moving the function (#2503981: Move checkEmptyRegions to named function and before first use (block.js))

nod_’s picture

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

Yeah it can be hard to stay on track. Please add a comment in #1415788: Javascript winter clean-up about that refactor so that we don't lose it. (may want to add a new "Refactor" section in the issue summary so it's not lost in the comments).

Technically context type is HTMLDocument|HTMLElement.
Use return not returns.
"Function to check empty" might be better as "Checks empty".

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Status: Needs work » Needs review
StatusFileSize
new jsdoc_block_module-2502629-6.patch5.16 KB
new interdiff3-6.txt1.65 KB

Technically context type is HTMLDocument|HTMLElement.
Use return not returns.

Fixed. I use @returns at work, so it probably slipped in there :)

"Function to check empty" might be better as "Checks empty".

Agreed.

Also found another 2 long lines that I fixed (not sure why i did not see them earlier).

nod_’s picture

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

Looks good to me :) thanks!

Fixes the 5 eslint jsdoc warnings.

jhodgdon’s picture

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

I patched Core and took a look at the two block module .js files. This looks pretty good but there are a couple of things I think still could/should be fixed:

  1. block.admin.js Drupal.behaviors.blockFilterByText
       * Text search input: input.block-filter-text
   * Target element:    input.block-filter-text[data-element]
   * Source text:       .block-filter-text-source

    This is probably going to format on a JSDoc site like this:

    Text search input: input.block-filter-text Target element: input.block-filter-text[data-element] Source text: .block-filter-text-source

    That is probably not what we want... so it should probably be made into a bullet list or three separate paragraphs?

    If it's a bullet list, our PHP standards require that it be preceded by an explanatory line ending in a : (which seems like a good idea).

  2. Two functions in block.admin.js do not have doc blocks at all:
    Drupal.behaviors.blockFilterByText :: attach
    Drupal.behaviors.blockHighlightPlacement :: attach
  3. Same for the attach functions in block.js
eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new jsdoc_block_module-2502629-9.patch6.83 KB
new interdiff-2502629-6-9.txt2.12 KB

1. Hm. First of all, you are entirely right about the output. It should not be like that. But I am not sure it should go in a bullet list, so I rewrote it to be separate paragraphs instead. Open to suggestions though.
2 and 3: Fixed.

eiriksm’s picture

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

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new core-jsdoc-block-2502629-11.patch6.44 KB

Reroll.

Using paragraphs look OK. We can improve later, let's get rid of the eslint warnings first.

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 10ecc78 on 8.0.x 
    Issue #2502629 by eiriksm, nod_, borisson_, jhodgdon: JSDoc block module

Status: Fixed » Closed (fixed)

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