[D7] TouchTouch

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxBlacksnipe2419583git

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.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

No: Does not follow the guidelines for 3rd party assets/code.
Would it be better if jquery.touchTouch is been added as library?

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

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 would recommend to use the libraries API for Javascript dependencies Libraries API 2.x

This review uses the Project Application Review Template.

You should also really have a hook_help() with some basic info about the module.

jQuery Touch Touch is an MIT-licensed plugin, and you have agreed to not host any code (PHP, JS, CSS) or anything else (images, etc) that does not comply with GPL v2 license. .
Consider resetting to clean up your repo at least.

You can always ask your users to download the plugins, or even better use libraries module for that.

It's the general practice to use Drupal behaviors to initialize Javascript functionality, so they work inside Ajax-tampared elements too.

Hi blacksnipe,

Please update your git information
I have found 'repository not found' error when going to clone using

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/blacksnipe/2419583.git touchtouch

1. The string "Enable the module" in help section should be removed, because user won't see this text if module will be disabled :) The same here.
2. Also, I think that code in hook_help() should be like this:
   

   /**
    * Implements hook_help().
    */
   function panels_contextual_help($path, $arg) {
     $output = '';
   
     switch ($path) {
       case 'admin/help#touchtouch':
         $help = array();
   
         $help[] = t("TouchTouch is a module that integrates the !plugin for Imagefields. It's a plugin that enables a popup, but with the difference that it supports responsive design, touch gestures and a bunch of other stuff. The markup provided is easier to theme and customize to what the site theme needs.", array(
           '!plugin' => l(t('jQuery-plugin by Martin Angelov'), 'http://tutorialzine.com/2012/04/mobile-touch-gallery/', array(
             'attributes' => array(
               'target' => '_blank',
             ),
           )),
         ));
   
         $help[] = '<h2>' . t('Usage') . '</h2>';
   
         $help[] = t('You can use this module as a !fieldformatter or a !viewsformatter.', array(
           '!fieldformatter' => l(t('field-formatter'), 'admin/help/touchtouch', array(
             'fragment' => 'field-formatter',
           )),
           '!viewsformatter' => l(t('views-formatter'), 'admin/help/touchtouch', array(
             'fragment' => 'views-formatter',
           )),
         ));
   
         foreach (array(
           'field' => array(
             'title' => t('Use in Views'),
             'items' => array(
               t('Edit the content type which contains the field you want to show as the TouchTouch-gallery.'),
               t('Manage the display of the content type (usually this is found under www.yoursite.com/admin/structure/types/manage/__your_content_type__/display)'),
               t("Change the format of the Image-field you'd like. Change it to TouchTouch gallery"),
               t('Select an image style for the clickable preview image'),
               t('... and the big image (not required)'),
               t('Save the settings'),
               t("That's it!"),
             ),
           ),
           'views' => array(
             'title' => t('Use in Views'),
             'items' => array(
               t('Add the fields or render the entities and follow the steps under Add as field formatter'),
               t('When selected as fields, set the formatter of the field to TouchTouch gallery'),
               t('Select an image style for the clickable preview image'),
               t('... and the big image (not required)'),
               t('Save the view'),
               t("That's it!"),
             ),
           ),
         ) as $type => $data) {
           // @see theme_item_list()
           $help[] = theme('item_list', $data + array(
             'type' => 'ol',
             'attributes' => array('id' => "$type-formatter"),
           ));
         }
   
         return '<div>' . implode('</div><div>', $help) . '</div>';
     }
   
     return $output;
   }
   

3. Would be better if you swap condition operands here. This is unnecessary, but it prevent from wrong variable assignment, when you forgot one equal sign. The same here.
4. The image_style_options() function called twice: here and here. Store the result into variable and use it.
5. Better to use !empty($items) here. Also, the loop should be put under this condition.
6. Very obvious comment :)
7. The drupal_add_js() should be invoked after libraries_load() returned TRUE.
   

   if (libraries_load('touchtouch')) {
     drupal_add_js('jQuery(function($) {$(".touchtouch").touchTouch();});', 'inline');
   }
   

You not able to bring back the PAReview: review bonus tag according to the rules of review bonus program of Drupal.org. To do that, you'll need to do three others manual reviews.

1. Why are you created this unnecessary variable? I've provided you a better example in comment #25.
2. Why do you put the break statement here? Are you know that nothing happens after return?
3. No need to compare types here. I've told you about that operands should be swapped. This is a best practice, used in Symfony, for example. The same here.

Think now it okay.

Now about operands. I've not told that you necessarily shall do that. This just a good practice, that prevent wrong variable assignment in condition.

For example, you just make a mistake and wrote:

$variable = 'example';

if ($variable = 'touchtouch') {
  // A little mistake, but can have big effects.
  db_drop_table('table');
}

In this case, condition will be interpreted as TRUE with all consequences. And otherwise, if operands swapped, you get the fatal error before the code will be executed :)

Thanks for your contribution!

I updated your account so you can 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 stay 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.