[D7] Scrollbar

The latest commit message indicates that this project has already been run through PAReview, which gives it a clean bill of health. I went through the code and can't find anything to complain about. My only suggestion right now is to clean up the minor formatting issues (some erroneously breaking lines) in the project page.

Just installed the module on fresh D7.

Module works like it should.

But you need to make a fix for the hook_menu() implementation.
The callback for the path admin/config/scrollbar is working, but it is not displayed on the admin/config page. You must choose a category.
For example: admin/config/user-interface/scrollbar
See https://drupal.org/node/549094#configuration for more informations.

All the other code looks fine.

And one suggestion for hook_uninstall():
For deleting so many variables I would prefer to use one call of:

db_delete('variable')->condition('name', 'scrollbar_%', 'LIKE')->execute();
cache_clear_all('variables', 'cache_bootstrap');

instead of variable_del()
Then you achieve the deletion of all your variables with only 1 MySQL query instead of 26 separate queries (one for each variable). It's better for the performance.

You have an PHP syntax error in the scrollbar.install file.
You forgot the (); on end of the line 46.

please fix this last issue.

Ok. Now everything is working like it should.
Testet again on a fresh D7.

About the JS improvements:
I would do it a complete different way. (here an example)
You should add all the settings to Drupal's global JS settings (Drupal.settings).
You can do it by refractor your scrollbar_inlineJS() function.

You can add the settings this way:

$scrollbar_settings = array(
    'showArrows' => variable_get('scrollbar_showArrows', TRUE),
    'mouseWheelSpeed' => variable_get('scrollbar_mouseWheelSpeed', 10),
    'arrowButtonSpeed' => variable_get('scrollbar_arrowButtonSpeed', 10),
    'arrowRepeatFreq' => variable_get('scrollbar_arrowRepeatFreq', 100),
    'horizontialGutter' => variable_get('scrollbar_horizontialGutter', 4),
    'verticalGutter' => variable_get('scrollbar_verticalGutter', 4),
    
    'verticalDragMinHeight' => variable_get('scrollbar_verticalDragMinHeight', 0),
    'verticalDragMaxHeight' => variable_get('scrollbar_verticalDragMaxHeight', 99999),
    'verticalDragMinWidth' => variable_get('scrollbar_verticalDragMinWidth', 0),
    'verticalDragMaxWidth' => variable_get('scrollbar_verticalDragMaxWidth', 99999),
    'horizontialDragMinHeight' => variable_get('scrollbar_horizontialDragMinHeight', 0),
    'horizontialDragMaxHeight' => variable_get('scrollbar_horizontialDragMaxHeight', 99999),
    'horizontialDragMinWidth' => variable_get('scrollbar_horizontialDragMinWidth', 0),
    'horizontialDragMaxWidth' => variable_get('scrollbar_horizontialDragMaxWidth', 99999),
    
    'arrowScrollOnHover' => variable_get('scrollbar_arrowScrollOnHover', 'false'),
    
    'verticalArrowPositions' => variable_get('scrollbar_verticalArrowPositions', 'split'),
    'horizontialArrowPositions' => variable_get('scrollbar_horizontialArrowPositions', 'split'),
    
    'autoReinitialize' => variable_get('scrollbar_autoReinitialize', 'false'),
    );
  
  if ($scrollbar_settings['autoReinitialize'] == 'true') {
    $scrollbar_settings['autoReinitializeDelay'] = variable_get('scrollbar_autoReinitializeDelay', 500);
  }
  
  drupal_add_js(array('jScrollPane' => $scrollbar_settings), 'jScrollPane');

After that the settings are accessible in the JS as Drupal.settings.jScrollPane
You simply can pass this object to the jScrollPane() function.

Although I wouldn't use a select field for TRUE || FALSE settings. Checkbox is better.

I would recommend to read this article http://drupal.org/node/751744 and sub articles.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hey,

First off, PAReview is complaining about there still being a master branch. Please follow http://drupal.org/node/1659588 to set the default branch, then follow steps 6 and 7 in http://drupal.org/node/1127732 to delete the master branch.

There are also some minor spelling mistakes in the README.txt (line 64 and line 17 are the ones I've spotted).

Manual review of code looks all good.

As per http://drupal.org/node/1410826 there are no major flaws so setting to RTBC

@TheodrorosPloumis you can review any project :)

Don't forget to add the "PAReview: review bonus" tag as indicated in #1410826: [META] Review bonus, otherwise you won't show up on my high priority list.

A few comments.

scrollbar.info

No quotes required in your .info file ... see http://drupal.org/node/542202

scrollbar.install

Line 29:
See http://drupal.org/node/322774 for the preferred way to include links in a translated string.

Line 40:
This has a high likelyhood of colliding with other module namespaces ... if you know the name of all variables, please use variable_del() on each variable explictly. Variable_del() also takes care of the cache and removing the variables from the global $conf array. (Also, I believe '_' is an SQL wildcard character, so this could also match 'scrollbars123' in addition to 'scrollbars_123'.)

scrollbar.admin.inc

Line 19, 222:
These lines should split up into multiple individual t() calls, one per sentence or <p> block, to help translators. Again, see http://drupal.org/node/322774 for details.

Line 33:
Typo on 'Advanced'.

Line 85:
Typo on 'Repeat'.

Line 118, 128, 138, 148, 158, 168, 178, 188:
The second sentence here is repeated multiple times in the subsequent elements. To simplify translations, it should be broken out into its own t() call in all of these elements, so that translators only have to translate it once. Also, <br /> is considered a block-level element, so it generally should not be included in the t() strings.

scrollbar.module

scrollbar_help():
Break out the block and link elements from your $missing1 and $missing2 messages, as per the above translations link. The "Lorem Ipsum Demo Content" text (label only) on the $lorem_ipsum message should be run through t().

scrollbar_init():
This function is run on every page ... at a bare minimum, I would place the requirements checks at the top of the function and not include the settings drupal_add_js() until you know that the required libraries exist.

None of the above are show-stoppers, though I highly recommend the t() changes.

Thanks for your contribution, TheodorosPloumis!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

- For the uninstall routine, the 'right' way would be to use variable_del() and list each of the variables individually. Don't use a db_delete() function at all, unless you can't know what the complete list of variables are (which is the case for modules that dynamically create variables for each content type, for example).

- No need to translate the Lorem Ipsum text.

- For the download link in scrollbar_help() (copied from http://drupal.org/node/322744):

We see a lot of bad examples in existing modules with links. A typical problem is that people try to remove HTML from the text and with it they of course also remove the link text itself. The link text then becomes isolated, not part of the flow of the sentence at all. This is a big problem for translation. Some developers use the ! placeholder because the link HTML code should not be escaped. This could be a problem if your external URL is not HTML-safe (eg. it includes an ampersand) as well. Finally, the translator has no idea that a link is happening behind the placeholder, let alone the text of the link, so translation combinations from such solutions tend to be funny instead of professional.

<?php
  // DO NOT DO THESE THINGS
  $BAD_EXTERNAL_LINK = t('Look at Drupal documentation at !handbook.', array('!handbook' => '<a href="http://drupal.org/handbooks">'. t('the Drupal Handbooks') .'</a>'));
  $ANOTHER_BAD_EXTERNAL_LINK = t('Look at Drupal documentation at <a href="http://drupal.org/handbooks">the Drupal Handbooks</a>.');
  $BAD_INTERNAL_LINK = t('To get an overview of your administration options, go to !administer in the main menu.', array('!administer' => l(t('the Administer screen'), 'admin'));

  // Do this instead.
  $external_link = t('Look at Drupal documentation at <a href="@drupal-handbook">the Drupal Handbooks</a>.', array('@drupal-handbook' => 'http://drupal.org/handbooks'));
  $internal_link = t('To get an overview of your administration options, go to <a href="@administer-page">the Administer screen</a> in the main menu.', array('@administer-page' => url('admin')));
?>

Hope this helps!