[D7] Super TOC

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi dillix,

I have test your module and take manual review, module worked, but have some bugs, also some suggestions.

• ESLint has found some issues with your code (please check the JavaScript coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

super_toc.js: line 6, col 1, Error - Missing "use strict" statement. (strict)
super_toc.js: line 18, col 36, Error - Missing space before function parentheses. (space-before-function-paren)
super_toc.js: line 20, col 10, Error - Extra space after key "width". (key-spacing)
super_toc.js: line 21, col 10, Error - Extra space after key "display". (key-spacing)
super_toc.js: line 23, col 58, Error - Expected '===' and instead saw '=='. (eqeqeq)
super_toc.js: line 28, col 21, Error - "hostname" is not defined. (no-undef)
super_toc.js: line 28, col 21, Error - "hostname" used outside of binding context. (block-scoped-var)
super_toc.js: line 28, col 32, Error - "pathname" used outside of binding context. (block-scoped-var)
super_toc.js: line 28, col 32, Error - "pathname" is not defined. (no-undef)
super_toc.js: line 28, col 43, Error - "qs" is not defined. (no-undef)
super_toc.js: line 28, col 43, Error - "qs" used outside of binding context. (block-scoped-var)
super_toc.js: line 28, col 48, Error - "hash" used outside of binding context. (block-scoped-var)
super_toc.js: line 28, col 48, Error - "hash" is not defined. (no-undef)
super_toc.js: line 29, col 34, Error - Missing space before function parentheses. (space-before-function-paren)
super_toc.js: line 31, col 12, Error - "hostname" used outside of binding context. (block-scoped-var)
super_toc.js: line 31, col 12, Error - "hostname" is not defined. (no-undef)
super_toc.js: line 32, col 12, Error - "pathname" used outside of binding context. (block-scoped-var)
super_toc.js: line 32, col 12, Error - "pathname" is not defined. (no-undef)
super_toc.js: line 33, col 12, Error - "qs" is not defined. (no-undef)
super_toc.js: line 33, col 12, Error - "qs" used outside of binding context. (block-scoped-var)
super_toc.js: line 34, col 12, Error - "hash" used outside of binding context. (block-scoped-var)
super_toc.js: line 34, col 12, Error - "hash" is not defined. (no-undef)
super_toc.js: line 37, col 12, Error - "hostname" is not defined. (no-undef)
super_toc.js: line 37, col 12, Error - "hostname" used outside of binding context. (block-scoped-var)
super_toc.js: line 38, col 12, Error - "pathname" is not defined. (no-undef)
super_toc.js: line 38, col 12, Error - "pathname" used outside of binding context. (block-scoped-var)
super_toc.js: line 39, col 12, Error - "qs" is not defined. (no-undef)
super_toc.js: line 39, col 12, Error - "qs" used outside of binding context. (block-scoped-var)
super_toc.js: line 40, col 12, Error - "hash" used outside of binding context. (block-scoped-var)
super_toc.js: line 40, col 12, Error - "hash" is not defined. (no-undef)
super_toc.js: line 42, col 14, Error - "pathname" is not defined. (no-undef)
super_toc.js: line 42, col 14, Error - "pathname" used outside of binding context. (block-scoped-var)
super_toc.js: line 43, col 16, Error - "pathname" used outside of binding context. (block-scoped-var)
super_toc.js: line 43, col 16, Error - "pathname" is not defined. (no-undef)
super_toc.js: line 43, col 35, Error - Expected '!==' and instead saw '!='. (eqeqeq)
super_toc.js: line 44, col 14, Error - "pathname" is not defined. (no-undef)
super_toc.js: line 44, col 14, Error - "pathname" used outside of binding context. (block-scoped-var)
super_toc.js: line 44, col 31, Error - "pathname" is not defined. (no-undef)
super_toc.js: line 44, col 31, Error - "pathname" used outside of binding context. (block-scoped-var)
super_toc.js: line 47, col 39, Error - Expected '===' and instead saw '=='. (eqeqeq)
super_toc.js: line 47, col 42, Error - "hostname" used outside of binding context. (block-scoped-var)
super_toc.js: line 47, col 42, Error - "hostname" is not defined. (no-undef)
super_toc.js: line 47, col 80, Error - Expected '===' and instead saw '=='. (eqeqeq)
super_toc.js: line 47, col 83, Error - "pathname" used outside of binding context. (block-scoped-var)
super_toc.js: line 47, col 83, Error - "pathname" is not defined. (no-undef)
super_toc.js: line 47, col 119, Error - Expected '===' and instead saw '=='. (eqeqeq)
super_toc.js: line 47, col 122, Error - "qs" used outside of binding context. (block-scoped-var)
super_toc.js: line 47, col 122, Error - "qs" is not defined. (no-undef)
super_toc.js: line 47, col 128, Error - "hash" used outside of binding context. (block-scoped-var)
super_toc.js: line 47, col 128, Error - "hash" is not defined. (no-undef)
super_toc.js: line 48, col 22, Error - "hash" used outside of binding context. (block-scoped-var)
super_toc.js: line 48, col 22, Error - "hash" is not defined. (no-undef)
super_toc.js: line 50, col 23, Error - "hash" is not defined. (no-undef)
super_toc.js: line 50, col 23, Error - "hash" used outside of binding context. (block-scoped-var)
super_toc.js: line 53, col 14, Error - "anchor" is not defined. (no-undef)
super_toc.js: line 53, col 14, Error - "anchor" used outside of binding context. (block-scoped-var)
super_toc.js: line 53, col 23, Error - "hash" used outside of binding context. (block-scoped-var)
super_toc.js: line 53, col 23, Error - "hash" is not defined. (no-undef)
super_toc.js: line 54, col 14, Error - "anchor" is not defined. (no-undef)
super_toc.js: line 54, col 14, Error - "anchor" used outside of binding context. (block-scoped-var)
super_toc.js: line 54, col 23, Error - "anchor" is not defined. (no-undef)
super_toc.js: line 54, col 23, Error - "anchor" used outside of binding context. (block-scoped-var)
super_toc.js: line 55, col 36, Error - "anchor" is not defined. (no-undef)
super_toc.js: line 55, col 36, Error - "anchor" used outside of binding context. (block-scoped-var)
super_toc.js: line 56, col 35, Error - Expected '===' and instead saw '=='. (eqeqeq)
super_toc.js: line 61, col 14, Error - "offset" is not defined. (no-undef)
super_toc.js: line 61, col 14, Error - "offset" used outside of binding context. (block-scoped-var)
super_toc.js: line 66, col 18, Error - "offset" used outside of binding context. (block-scoped-var)
super_toc.js: line 66, col 18, Error - "offset" is not defined. (no-undef)
super_toc.js: line 69, col 18, Error - "offset" is not defined. (no-undef)
super_toc.js: line 69, col 18, Error - "offset" used outside of binding context. (block-scoped-var)
super_toc.js: line 73, col 16, Error - "offset" is not defined. (no-undef)
super_toc.js: line 73, col 16, Error - "offset" used outside of binding context. (block-scoped-var)
super_toc.js: line 78, col 16, Error - Extra space after key "scrollTarget". (key-spacing)
super_toc.js: line 79, col 16, Error - Extra space after key "offset". (key-spacing)
super_toc.js: line 79, col 25, Error - "offset" is not defined. (no-undef)
super_toc.js: line 79, col 25, Error - "offset" used outside of binding context. (block-scoped-var)
super_toc.js: line 88, col 134, Error - Missing semicolon. (semi)
super_toc.js: line 91, col 10, Error - "visibility_hide" used outside of binding context. (block-scoped-var)
super_toc.js: line 94, col 10, Error - "visibility_hide" used outside of binding context. (block-scoped-var)
super_toc.js: line 94, col 28, Error - "visibility_hide" used outside of binding context. (block-scoped-var)
super_toc.js: line 94, col 44, Error - Expected '===' and instead saw '=='. (eqeqeq)
super_toc.js: line 96, col 91, Error - "visibility_hide" used outside of binding context. (block-scoped-var)
super_toc.js: line 97, col 12, Error - "visibility_hide" used outside of binding context. (block-scoped-var)
super_toc.js: line 97, col 28, Error - Expected '===' and instead saw '=='. (eqeqeq)
super_toc.js: line 101, col 45, Error - Missing space before function parentheses. (space-before-function-paren)
super_toc.js: line 109, col 20, Error - Extra space after key "path". (key-spacing)
super_toc.js: line 114, col 20, Error - Extra space after key "expires". (key-spacing)
super_toc.js: line 115, col 20, Error - Extra space after key "path". (key-spacing)
super_toc.js: line 123, col 12, Error - Expected a "break" statement before "default". (no-fallthrough)
super_toc.js: line 123, col 77, Error - Unnecessary semicolon. (no-extra-semi)
super_toc.js: line 129, col 20, Error - Extra space after key "expires". (key-spacing)
super_toc.js: line 130, col 20, Error - Extra space after key "path". (key-spacing)
super_toc.js: line 135, col 20, Error - Extra space after key "path". (key-spacing)

94 problems

Bugs

1. Clean empty values of checkboxes.

This bug will be happen on front-end, exclude "Heading levels" still display without "Show hierarchy ", also html still here with "Show hierarchy ".

super_toc.module (line 231)

<?php
// current
$heading_levels = variable_get('super_toc_heading_levels', $heading_levels_defaults);
// new
$heading_levels = array_filter(variable_get('super_toc_heading_levels', $heading_levels_defaults));
?>

2. Same id of span tag when same title

An article may be have some text is same in "Heading levels", then the id of span and hash tag on link will be same, so goto hashtag will be incorrect when click TOC link.

Suggestions

1. In file super_toc.admin.inc

<?php
  $options = array(
    1 => t('Before first heading (default)'),
    2 => t('After first heading'),
    3 => t('Top'),
    4 => t('Bottom'),
?>

For super_toc_position, I think you can define Constant variables, then replace (1,2,3,4) with Constant variable, such as SUPER_TOC_POSITION_BEFORE, SUPER_TOC_POSITION_AFTER, SUPER_TOC_POSITION_TOP, SUPER_TOC_POSITION_BOTTOM, also update this on if conditions in file super_toc.module.

<?php
  $options = array();
  for ($i = 2; $i < 11; $i++) {
    $options[$i] = $i;
  }
?>

Drupal have function to do that, drupal_map_assoc(array(2, 3, 4, 5, 6, 7, 8, 9, 10)).

2. Add validate and set values to NULL if not visible for "#state"

"super_toc_smooth_scroll_offset" will be visible when "super_toc_smooth_scroll" checked, so it is good to add form validate function to check "super_toc_smooth_scroll", if not checked, "super_toc_smooth_scroll_offset" value should be NULL, also for other "#state".

3. Clean empty values of checkboxes

super_toc.module (line 39-41)

<?php
// current
      $nodetypes = variable_get('super_toc_nodetypes', array());

    if (isset($nodetypes[$entity->type]) && !empty($nodetypes[$entity->type])) {

// new
        $nodetypes = array_filter(variable_get('super_toc_nodetypes', array()));

    if (isset($nodetypes[$entity->type])) {
?>

Hi dillix,

Good job! There were no pareview issues, and all #3 bugs fixed.

But I'm so sorry, found below, please check and test again.

1. I'm try to config Show text to "Show >" and Hide text to "Hide >" in admin settings page, then show worked, but hide trigger not working.

2. You have used Constant variables for super_toc_position, so I think you should update variable_get('super_toc_position', 1) to variable_get('super_toc_position', SUPER_TOC_POSITION_BEFORE) in files (super_toc.module, super_toc.admin.inc).

3. Lines 153-157 of file super_toc.admin.inc

<?php
    '' => t('Grey (default)') . '<p><img src="/' . $module_path . '/images/grey.png" /></p>',
    'toc_light_blue' => t('Light blue') . '<p><img src="/' . $module_path . '/images/blue.png" /></p>',
    'toc_white' => t('White') . '<p><img src="/' . $module_path . '/images/white.png" /></p>',
    'toc_black' => t('Black') . '<p><img src="/' . $module_path . '/images/black.png" /></p>',
    'toc_transparent' => t('Transparent') . '<p><img src="/' . $module_path . '/images/transparent.png" /></p>',
?>

Suggest to use theme('image', $variables) to replace part of img.

Hi dillix,

Great, I've tested, and all are worked.

I think this module is RTBC? Anyone else?

Hi dillix,

This is a very interesting module: I installed it and it's working as expected, so I'll be glad to set status to RTBC, but there are currently a few points against that. This isn't at tall to say your project is bad, but it souhld conform to the Drupal community's policies.

Here is my review based on the usual review template:

Automated Review

I'd not looked at PAReview since everright said it's ok, but please note that to be consistent your task page should include a link to PAReview results.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

[Yes: Does not cause / No: Causes]??? module duplication and/or fragmentation.

This sounds like a feature that should live in several existing projects. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition.

This is a point you didn't address in your project page. Could you explain how this module differs notably from those listed in this TOC modules comparison page?
You may look at the "Similar projects and how they are different" paragraph in the Tips for a great project page page.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the README Template.
You should at least populate the "Requirements" section like in your project page.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

I didn't really dive deeply in the code till now, but at a first glance it looks clean. Here are only points I noticed as a user. Note that only the first one can be considered as a real issue and should be improved.

1. (*) In each node you only look at $entity->content['body']['0']['#markup']: so if the body field has more than one value, only the first one is processed.
   In some cases, especially for long articles (which is just your module's target usage), a body field may be split in a number of values, each one standing for a kinda <h1> and afterwards including lowest <h?> parts.
   So you should at least process any existing $entity->content['body']['**n**']['#markup']
2. In such a situation (where a long node is made of several body values), the configuration form could offer an interesting option to the user:
   • - Have a TOC inside each body instance
   • - Or have a general TOC for the whole body instance
3. Also note that currently only body field is involved: you may encounter nodes where other, user-defined fields are of type long-text too, so it might be consistent to process them as well.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.