Lights Out

Please add git link as "http://git.drupal.org/sandbox/jcamposanok/1659612.git" and also drupal version in the Issue summary.
And remove minor issue given by ventral.org here.

I tried the module on a fresh install with Bartik and initially could not get it work. The fade in/out effect works fine, and the block view of the switch also functions correctly. However, I did not see the region I had selected being highlighted in any way. I checked the lights_out.js file and discovered that on line 5:

      var region = '#region-' + Drupal.settings.lights_out_region;

The # hash mark is hardcoded into the jQuery class. Region names are being output as classes, not IDs, so this was not correctly detecting my regions. When I changed it to...

      var region = '.region-' + Drupal.settings.lights_out_region;

... it began working as expected (a "highlight" class was added to the selected region, and its z-index was changed to 200, while the overlay z-index was set at 100). However, despite the fact that the the z-indexes were correct, in Firefox 14 and Chromium 18 the region still appeared to be behind the overlay. I think this is due to the fact that z-indexes are basically ignored by the rendering process unless the elements (or their parents?) are also positioned absolutely or relatively.

I imagine the use case for this module would be to highlight some element in the middle of the screen (such as a video player, which would be positioned absolutely), but out-of-the-box this module will only function as intended if your chosen theme uses absolute positioning for its regions.

Manual review focusing on Javascript:

lights_out.css

1. .hilight/#shadow are very generic names, it is highly likely that other modules/themes may also be applying CSS to these selectors. I'd suggest prefixing with 'lights_out_'.

lights_out.js

1. 19-22: To default 0 if NaN, can be simplified to single line var zInd = $(this).css('z-index') || 0;
2. 27-29, 46-48: Consider moving this to one function, simplifying by using .toggleClass() for both classes and chaining on '.lights_out_switch a' so you're not incurring multiple DOM traversals. I.e.
   

   toggle_switch: function () {
     var switch_anchor = $('.lights_out_switch a');
     var switch_anchor_label = Drupal.settings.lights_out_label_off;
     if (switch_anchor.text() == Drupal.settings.lights_out_label_off) {
       switch_anchor_label = Drupal.settings.lights_out_label_on;
     }
     switch_anchor.toggleClass('is-off').toggleClass('is-on').text(switch_anchor_label);
   }
   

   This also means lines 6/7 no longer needed.

3. 14,32: Split shadow_on and shadow_off functionality into their own behaviours to improve readability of attach(). I.e.
   

   switch (state) {
     case 'is-on':
         Drupal.behaviors.lights_out.shadow_off(region);
       break;
     case 'is-off':
         Drupal.behaviors.lights_out.shadow_on(region);
       break;
   

   Line 57 can now also be Drupal.behaviors.lights_out.shadow_off(region); instead of faking a click.

   This also allows other modules to trigger lights on/off behavior (i.e., a video player module may want to integrate!)

4. 18-24, 39-45: Could move this into a custom jQuery method function. I.e.
   

   $.fn.adjustZIndex = function(adjustment) {
       this.each(function(index){
         var zInd = $(this).css('z-index') || 0;
         $(this).css('z-index', zInd + adjustment);
       });
     return this;
   };
   // Usage:
   $(region).find('*').adjustZIndex(200);
   $(region).find('*').adjustZIndex(-200);
   

5. 61: JSLinter: Missing semicolon (after closing brace for Drupal.behaviors.lights_out assignment)

Tested and working well on 7.16-dev. Nifty little module!

Would also be cool if there was a way to have a user uploaded shadow .png (i.e. a lower opacity black) to make it even darker, or perhaps to upload a png with watermarks, or who knows!

Handy little module this...

Few points.

1. Add a hook_help()
2. Consider using a theme function rather than having HTML in your module file. eg Line 126 where you build your block. I know it's not much but using a theme function will also allows other modules to change the output.
3. Maybe the directory structure could be tidied up. Eg, create css, js and image folders. It helps keep everything tidy as your module expands.

Other than that looks good.

Manual review
------------------
1. Try to adhere to this style of writing readme.txt http://drupalcode.org/project/bot.git/blob/HEAD:/README.txt
2. Within .info file it would be better to specify both package and configure parameters.
3. Be consistent with usage of functions that serve the same purpose. The .install file uses both get_t() and st() functions. Better just use st() as .install file is used only during installation/uninstall phases and st() function is just for that.
3. I am not sure for what purpose you are using lights_out_admin_validate function within lights_out.module. Currently, this function seems to create problem when trying to save Region having "_" within region's machine name. Thereby not saving the Region selected. May be you had a special case within the theme you used when creating this module.

1) README.txt is quite sparse. I think including the main use case (video watching?) with some links to demo versions of the 'Lights Out' effect would be nice.

2) Lights out link didn't display for my chosen content type that was being displayed in Views block. The block link displays though. However, wherever the link did show up, it worked dimming all the regions that weren't the region to be highlighted.

Code looks good to me. PAReview showed up clean. I think this will be a nice addition to Drupal!

Hi jcamposanok,

There is some commenting errors as instead of "Implements of" use "implements" in the lights_out.module file otherwise the module code looks good to me.

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 :-)

Everything looks good here and no security issues as far as I see, but:

1. The constants strings used as default values are not translatable.
You can use t() to avoid that.

define('LIGHTS_OUT_REGION', 'content');
define('LIGHTS_OUT_LABEL_OFF', 'Turn off the lights!');
define('LIGHTS_OUT_LABEL_ON', 'Turn on the lights!');

For example adding as isolated lines

t('Turn off the lights!');

2. Why its the window position moving after click on the lights on link ?
It wouldn't be nicer to let the animation happen without that jump ?

Please can you take care 1 & 2 ?
I think those are not blocker issues so I guess this should be approved.

Thanks logicdesign for the additional review.

manual review:

1. project page is a bit short, see http://drupal.org/node/997024
2. lights_out_preprocess_page(): do not use that hook, which is invoked for every page. You should add your stuff only when the links are actually displayed, which is in lights_out_block_view() and lights_out_node_view().

But that are not application blockers, so ...

Thanks for your contribution, jcamposanok!

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.