[D7] Overlay Light

Welcome,

please get a review bonus first. Then try to fix issues raised by automated review tools: http://ventral.org/pareview/httpgitdrupalorgsandboxgeorgemastro1842116git

Hello,

i think it's better when you use the drupal.behavior functionality. Your JS can look like this:

(function ($) {
  Drupal.behanviors.disable_overlay = {
    attach: function (context, settings) {
      var path = Drupal.settings.disable_overlay.path;
      var basepath = Drupal.settings.disable_overlay.basepath;
      if ($('.tabs').length > 0) {
        $('.tabs').append('<li><a href="' + basepath + path + '">Close</a></li>');
      }
      else {
        $('#content').prepend('<a href="' + basepath + path + '" id="overlay-close">Close</a>');
      }
    }
  }
})(jQuery);

You can read about it here: http://drupal.org/node/304258

I agree with the above in that you should be using drupal behaviors (though watch out for the typo in the above code)

installing the module:
installing the module seemed to work fine and disabled the overlay module and took me back to the modules page. This page now had a 'close' tab, selecting this then took me to 'http://localhost/d7/js/admin_menu/cache/7175c7148b3e6c07541b488a6c32d279'. I am not exactly sure what's going on here.
Going back to the front page, then the modules page again, the 'close' tab now pointed to the front page as expected.

Turning off the module did not re enable the overlay module. Maybe your module could re enable the overlay when you module is disabled. Maybe save the overlay module status when you module is enabled, then read this when you module is disabled to determine if the overlay should be re enabled or not. Just a thought and maybe not necessary.

When I enabled you module for the second time, I was redirected to the front page rather than the modules page.

Using the module
I find that in some cases the 'close' button does not work as expected.
eg. I go to the content page '/content' and then click and edit link on a node 'node/1/edit', clicking the 'close' link on this edit page takes me back to the front page. It seems you module is not reading the destination get parameter in the url. Maybe incorporate the drupal_get_destination function in there somewhere.

Code
The small amount of code in this module all looks fine to me (except for the behaviors issue outlined above)

The Project application checklist has a section titled 'Ensure your project contains a minimum of handwritten code.', you may want to review this.

Hi georgemastro,

There is no need to write $message = t() inside drupal_set_message() at line 37 in file disable_overlay.module and line 12 in file disable_overlay.install(). You can use as drupal_set_message(t("Your message")). For more information visit http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drupa... and look at the usage of drupal_set_message().

Thanks,
Udaksh

Hi,

Isn't still better to just disable the Overlay core module?

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1929998
Project 2: http://drupal.org/node/1856530

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects.

And please add all your manual reviews to the issue summary as indicated in #1410826: [META] Review bonus.

Manual Review:

disable_overlay_enable
You don't need to output the enabled message, this is done by Drupal when it's enabled. (line 12)
exit - produces an error when enabling in Drush. Remove this line.

disable_overlay_disable
You don't need to output the enabled message, this is done by Drupal when it's disabled. (line 21)

Just after I enable module, I got an error:
Fatal error: Call to undefined function drush_verify_cli() in E:\test_site\sites\all\modules\disable_overlay\disable_overlay.install on line 13

Also if your have some AJAX on your admin page, overlay icon duplicates on each AJAX reload. See attached screenshot.

And another issue: it break views interface when moduel enabled. See attached screenshot.

• When user creates a new article, the overlay-close graphic gets duplicated 3 times.
• Small clipping error on some pages, make the close button into a transparent png graphic.
• When disabling the module I received the message 'The overlay module was disabled.' when it should be 'Disable Overlay module was disabled'.

I believe we need an answer to this comment:

Isn't still better to just disable the Overlay core module?

To me, this seems like a far easier solution. What makes your module better than just disabling the Overlay module?

The issues in #11 need to be fixed before this can be RTBC. These will generate error on enable without drush installed.

In function disable_overlay_init(), only set modules_exists return to a variable if you plan on using it more than once.

if (module_exists('overlay')) {
  module_disable(array('overlay'));
  drupal_set_message(t('The overlay module was disabled.'), 'status');
}

Hmm, this module is a little confusing. I would describe it more as "Instead of using overlay, this module adds the close/navigation functionality without a complete (heavy) overlay." If you want to be sure overlay.module is not enabled use a hook_requirements() to let the admin know there's an issue. Possibly you could fake it with dependencies[] = overlay (<1.0) which should always fail. That should make hook_init() simpler, and I think you can remove disable_overlay.install entirely.

Maybe a better name for the module would be "Overlay Light" or Simple?

----
Top Shelf Modules - Enterprise modules from the community for the community.

Updated description.

Hi,
Please fix ventral.org issues :

FILE: /var/www/drupal-7-pareview/pareview_temp/css/overlay_light.css
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
14 | ERROR | CSS colours must be defined in lowercase; expected #ffffff but
| | found #FFFFFF
38 | ERROR | Additional whitespace found at end of file
40 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/overlay_light.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
13 | ERROR | Case breaking statements must be followed by a single blank line
--------------------------------------------------------------------------------

Thanks

..

Changing status to needs work

That minor issues reported by automated review tools are not application blockers, please do a manual review.

Thanks for your feedback, I guess that's a reasonable implementation. I reviewed the current codebase again, everything looks good except

Code too short

This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Hi,

Overlay_light is a very nice module and yes, responds much faster than Overlay. However, I found something that needs some explanation :
> If initially Overlay is disabled and i enable Overlay_light, everything seems to be working fine and perfect. Ok. Now if i enable Overlay module, i cannot use it. Shouldn't Overlay_light get disabled when Overlay gets enabled and vice versa..... In short, if i could just switch between them automatically.

.install file
line 22
> Suggestion : You can write less code here :

  if (module_exists('overlay') == FALSE) {
    module_enable(array('overlay'));
    drupal_set_message(t('The overlay module was enabled.'));
  }

instead of :

  $overlay = 'overlay';
  $overlay_module_enabled = module_exists($overlay);
  if ($overlay_module_enabled == FALSE) {
    $module_list = array($overlay);
    module_enable($module_list);
    drupal_set_message($message = t('The overlay module was enabled.'), $type = 'status');
  }

.module file
line 30
You can write less code here too, as discussed above.

> Whenever i enable or disable Overlay_light, i never see any message : neither 'The overlay module was enabled.' nor 'The overlay module was disabled.' when Overlay was already enabled.

okay, so with this all seems well then, changing status to RTBC
- good luck

This one is already RTBC. As you should not have more than one application, it is wise to stick to Overlay Light for now and meanwhile get a Review bonus again.

The Commerce Viva Payments sandbox module looks well commented in the code. It looks that it went through an automatic review process already (PaReview, Coder?). Have no opportunity to test, as I don't use the gateway.

That's good enough for me that other module looks decent.

Thanks for your contribution, georgemastro!

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.

----
Top Shelf Modules - Enterprise modules from the community for the community.