Image Preloader

welcome

There is still a master branch, make sure to set the correct it and Drupal Code Sniffer has found some issues with your code you can check here http://ventral.org/pareview/httpgitdrupalorgsandboxdevelopmenticon180542...

first attempt to install this module work fine with the settings. Miner issue, i had list in the front page by "Promoted to front page" where the effect is not happening...

do to other project application review and get bonus to finish it faster.

The functionality on the demo looks cool, it would be a nice module.

There are some issues with the code though, asside from the fact that it doesn't always follow the Drupal coding standards.

General

• There is a dependency on views, as the functionality will also work on blocks and content types adding this dependency is not ideal. You should check in your code in the appropriate places if the views module is available.

image_preloader.info

• The files defined in the files[] directive are not required, you should remove them
• There is no package defined, try to find or think of one that matches your module the best

image_preloader.module

• The rule name could be better, follow the general naming convention: administer image_preloader
• The php array('$default_theme') is incorrect, you cannot place a variable in single qoutes. In general, avoid placing variables in quotes at all. The code becomes less readable
• It seems unnecessary to include the default theme in the callback, you could just as easily load it in the callback itself
• Try to use a different approach than the hook_page_build, maybe an entity/node hook, views hook etc? This way you will not have to check for the arg(0)
• You can assume jquery is available, you don't need to check for it
• You call drupal_add_js twice for a image_preloader setting. These two calls should be merged into 1
• There is actual javascript code in the last drupal_add_js, where it shouldn't be. Add this to an external file, I don't see any problem with adding it to the existing JavaScript file
• The best way, if possible to access JavaScript object properties is via the dot, ["image_preloader_id"] could be changed to .image_preloader_id
• The JavaScript uses a jquery event instead of Drupal behaviors, it should be changed to use Drupal behaviors

image_preloader.admin.inc

• The admin_display method uses a 'private' function _block_rehash, is it possible to rewrite not using this function?
• There is an unused variable: $views_name
• $blocks = $blocks; Seems unnecessary
• Same with $image_preloader_icons = $image_preloader_icons;

image_preloader.js
In general, wouldn't it be better to attach a load event on each image and remove the loader class then? As far as I can see it would require only a small amount of logic instead of the setInterval logic currently used.

• The code is a bit messy, please follow the Drupal coding standards
• You defined variables in 1 go, but continue to define the init function, this is not very clean. Define it seperately
• Use a defined function at line 44 instead of creating a new function each time you want to remove the image-preloader-icon class

I hope it helps you a bit!

Not all comments I gave are in yet but a lot of them are, it looks a lot better already! It would be better to look at the other issues I metioned about the other JavaScript approach and the replacement of the hook_page_build.

I took a quick look and it seems there is a block_list function which will return the blocks for a certain region. You can in turn return all regions with the system_region_list.

$blocks = array();
foreach (array_keys($all_regions) as $region) {
  $blocks[] = block_list($region);
}

or maybe

$blocks = array();
foreach ($all_regions as $key => $region) {
  $blocks[] = block_list($region);
}

Though I have not looked at your module and am just posting this based on your post above.

Check that all the variables are what you expect them to be.
I would start with $all_regions and make sure it is going through the loop.
If you are not already using it, I would suggest using the devel module. You can then use dpm($all_regions); to get a pretty print out of the object.

Hi,

I look like a very nice project that probably will help many drupal users.

The automatic test looks just fine
If you changed to Pereview, you should place in the issue initial description the list of reviewd projects comments

Please notice several issues that may consider to take care of:
1. at image_preloader_page_build(), I've not seen arg decleration. Yet, it recommended to use consts for arg(0), so even a newbie will understand the meaning of the code
2. Refactor image_preloader_page_build() to several single purpose function.

Best Regards,
Moshe

Removing review bonus tag, no reviews of other projects listed in the issue summary. Please read #1410826: [META] Review bonus.

Sweet module, especially pacman!

manual review:

1. "'access arguments' => array('access administration pages'),": The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations. Why don't you use the permission that you define?
2. image_preloader_init(): if you just want to add CSS on every single page request you should define that in your info file. See http://drupal.org/node/542202
3. "variable_get('image_preloader_posts_' . $nodetype->type . '_enabled')": if your module defines variables they should be removed in hook_uninstall().
4. image_preloader_icon_list(): do not create image markup yourself, use theme('image', ...) instead.

Otherwise this looks nearly ready, but I consider especially the permission problem a blocker. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Added Reviews of other projects

Looks good, all of the suggestions appear to have been implemented.

manual review:
"modified image_preloader.install" is not a useful git commit message, I can see that from the diff. You should describe what you changes, see also http://drupal.org//node/52287

Thanks for your contribution, developmenticon!

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.