Power Zoomer

- please add t() function for this line

    'title' => 'Power Zoomer settings page',

- Although your js is quite clear, please make it follow Drupal coding standard to be able to pass this review.
- And please fix all coding standard errors as attachment.

The settings page could use a better description and might should conform to the standard language and pattern.

Right now:

Power Zoomer settings page

Proposed Change:

Power Zoomer
Manage the power zoomer settings.

Patch attached.

With the module enabled, I can't actually get it to work either:

Failed to load resource: the server responded with a status of 404 (Not Found) http://localhost:8888/drupal-7.15/sites/all/modules/power_zoomer/ddpower...

This is a fresh drupal-7.15 site.

Hi tm01xx,

I installed your module without a hitch. Minor point however, far from 'needs work' as this is simply a suggestion.

Would you consider hooking the Library module to implement your JS dependency? Maybe something like this in your powerzoomer_init().
$js = libraries_get_path('powerzoomer');

function powerzoomer_library() {
  $libraries['powerzoomer'] = array(
    'title' => 'ddpowerzoomer.js',
    'website' => 'http://',
    'version' => '',
    'js' => array(
       drupal_get_path('module', 'powerzoome') . '/ddpowerzoomer.js' => array(),
    ),
  );
  return $libraries;
}

If your path is not set you could then provide the administrator a message urging them to download the required JS file. This could further lead onto hook_requirements.

Cheers,

Hi,

I went through your code, but noticed this on line 97:

drupal_add_js(drupal_get_path('module', 'powerzoomer') . '/ddpowerzoomer.js');

When I follow the installation steps, ddpowerzoomer.js is not placed in the module folder, but in the sites/all/libraries/powerzoomer folder.

Therefore you would need to do something like this:

$jsdir = libraries_get_path('powerzoomer');
drupal_add_js($jsdir . '/ddpowerzoomer.js');

cheers!

Oh and as said in #6, create an .install file with hook_requirements() and check for the 'ddpowerzoomer.js' file on install or during runtime so you can display a message when the file is not present.

hook_requirements()

The latest from git: 74157a1aa

Puts the libraries in sites/all/modules not sites/all/libraries

As you're requiring external libraries, it would be beneficial for all users (also noob users) that they got a notice on install when the required libraries are not correctly installed. This seems to be common to all modules requiring libraries.
An example of such an .install file with library requirements can be found in the jw player module: http://drupal.org/project/jw_player

Just my thought...

Hello,

there is an inconsistency in the location of the js file. In your README.txt file you say that the users should place the js file in sites/all/libraries/powerzoomer/ddpowerzoomer.js, but your .module and .install file both refer to (sites/all/)modules/powerzoomer/ddpowerzoomer.js.

So first make sure the user (and yourself) places ddpowerzoomer.js in sites/all/libraries/powerzoomer/.

Some other notes. You don't need the hook_library() from core but hook_libraries_info(), which is a hook provided by the Libraries API.

That would look like this in powerzoomer.module:

/**
* Implements hook_libraries_info().
*/
function powerzoomer_libraries_info() {
  $libraries['ddpowerzoomer'] = array(
    'name' => 'Powerzoomer library',
    'vendor url' => 'http://www.dynamicdrive.com/dynamicindex4/powerzoomer.htm',
    'download url' => 'http://www.dynamicdrive.com/dynamicindex4/ddpowerzoomer.js',
    'version' => '1.1',
  );
  return $libraries;
}

Then in your powerzoomer.install file put these two functions (I adapted it from my Jmol module to yours, check again for typos and errors please):

/**
 * Implements hook_install().
 */
function powerzoomer_install() {
  $requirements = powerzoomer_requirements('install');
  $message = $requirements['powerzoomer']['value'];
  if ($requirements['powerzoomer']['severity'] == 2) {
    $messagetype = 'error';
  }
  else {
    $messagetype = 'status';
  }
  drupal_set_message($message, $messagetype);
}

/**
 * Implements hook_requirements().
 */
function powerzoomer_requirements($phase) {
  $requirements = array();

  // Ensure translations don't break at install time.
  $t = get_t();

  drupal_load('module', 'libraries');
  $directory = libraries_get_path('ddpowerzoomer');

  // Check the existence of the powerzoomer Library ddpowerzoomer.js file.
  if (($phase == 'install') or ($phase == 'runtime')) {
    $errors = array();

    if (!file_exists($directory . '/ddpowerzoomer.js')) {
      $errors[] = $t('The file %file is not present, indicating that the powerzoomer library is not correctly installed.', array('%file' => 'ddpowerzoomer.js', '%directory' => $directory));
    }

    $requirements['powerzoomer'] = array(
      'title' => $t('Powerzoomer'),
      'value' => !empty($errors) ? theme('item_list', array('items' => $errors)) . $t('Please consult README.txt for installation instructions.') : $t('Powerzoomer has been installed correctly. You are ready to go.'),
      'severity' => !empty($errors) ? REQUIREMENT_ERROR : REQUIREMENT_OK,
      // Adding a description key suppresses an error during install,
      // see @link http://drupal.org/node/1312636 @endlink.
      'description' => '',
    );
  }
  return $requirements;
}

The install file makes sure that your module can only be enabled when the ddpowerzoomer.js file is in the correct place. It displays an error upon install when this is not the case. It also reports (runtime) an error in the Reports section of your Drupal site when the file is not installed or not in the correct place.

cheers!

Ok sure, but then one of these lines is obsolete in powerzoomer.module:

  drupal_add_js(drupal_get_path('module', 'powerzoomer') . '/powerzoomer.js');
  drupal_add_library('powerzoomer', 'ddpowerzoomer');

They both add the javascript file if I'm not mistaken.
I think that it is best to also name your parent folder powerzoomer and not power_zoomer. The module is called powerzoomer, so should the folder.

Now I tried to use the module but I can't get it to work. I read this in your README:

I. FOR QUICK USE

1. Visit the settings page and enter page you want to use 
this powerzoomer for your images. Enter * for all pages.
(sites/all/modules/powerzoomer)

2. In your images, please add class name "powerzoomer" per image.

3. Reload the page and mouseover on the image.

Now as a normal user, I made a new content type with an Image field and added content to it (an image). But I have no clue where to add a class name. You really need to be more specific here.

And I still think you should use the Libraries API when you are not the creator of the library.
As noted here: http://drupal.org/project/libraries
But I will leave this up to another reviewer.

Honestly, I consider myself a developer and I know HTML and PHP. I still have no idea where to add the image class name. I haven't used lightbox in a while, but I don't remember adding an image class name there.

Okay, my concept was that it would work with the image field included with core. Would it be possible to integrate this with your module?

I really thought this was how the module is supposed to work. A module like this shouldn't force the user to write code.
I think the best way to go would be to create an Image field formatter to display zoomable images.

To accomplish that you should use these hooks:

hook_field_formatter_info()
hook_field_formatter_settings_form()
hook_field_formatter_settings_summary()
hook_field_formatter_view()

You could take a look at my module where I made a File field formatter to display a special kind of file in a Java applet.

Changed status.

Yeah, exactly! This would give your module a greater general appeal and make it much more simpler to use. I really like the concept of the module too and I think it would be a great addition.

I think you've nailed it this time!

I used just the core article content type and it worked beautifully. This is great. RTBC for sure.

Hi there,

yep, this works nicely. I do have to report a conflict with the Admin module, but that's a jQuery Update bug, so you shouldn't worry about it, the jQuery Update maintainer has to fix this (I reported an issue there). But I would mention it on the project page and in the README.txt. When the Admin module is enabled, the powerzoomer doesn't work.

Some minor notes:

Your README.txt refers to a folder called "powerzoomer", but your git code contains a "power_zoomer" folder. Better to rename all of this to "powerzoomer" for consistency.

Power range high value: is there a maximum value for this? Can people actually enter 1000000?

In README.txt point 3 for normal users, modify to: "3. Click on the cog wheel and select an ...". This is more clear for new Drupal users.

Well done mate. Good module! :)

Your project name is pretty much set in stone now, at least for the sandbox module. When you get full project access, you'll have the opportunity to make the url anything you want: drupal.org/project/power_zoomer

I don't think you can delete your sandbox either, you couldn't the last time I checked. You could create a new sandbox and use the name you want for your project folder and then just update the links in your project application.

For #2, I wonder if it might be a good idea to limit the value to at least some reasonable maximum value? I also wonder how this will work with responsive sites?

For your next step, do three more PAReviews and Klausi will check in a week and you'll get full-project access if he doesn't find any issues with your code.

Ok, nevermind the folder name...
I would however set a maximum value of 10 then.

cheers!

If you review three other projects, you can add a bonus tag and an admin will come to your project much faster for approval.
See here: http://drupal.org/node/1410826

Just took a look through the repository, and had the following comments:

7.x-1.0 branch

The branch name "7.x-1.0" does not match Drupal's release naming conventions. See http://drupal.org/node/1015226. Non-standard branch names are allowed within a repository, but can not be used to generate a release on Drupal.org.

powerzoomer.install line 34

$errors[] = $t('Powerzoomer requires library <a href="@url"> ...
I would suggest pulling the 'a href' tag out of the translatable string, and passing a full l() function as the content of the @url placeholder.

Libraries API

I would strongly, strongly recommend leveraging the Libraries API to include the jquery library, as mentioned by other reviewers. While it does seem unlikely that two modules would be installed which both leverage the ddpowerzoomer.js library, this is much more likely for other ultility type jquery libraries ... and for consistencies sake, we try to enforce the same approach for ALL implementations.

Otherwise, looks great! No showstoppers, so ...

Thanks for your contribution, tm01xx!

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.