node_overlay

Welcome,

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

This module is quite good, my suggestion in this module is that we can use drupal standard coding guideline mentioned in http://drupal.org/coding-standards for control structure which is used in node_overlay.module in line number 35

Line 35: Control statements should have one space between the control keyword and opening parenthesis
if($menu_item && (isset($menu_item['page_arguments'][0]))) {

It can be written as
if ($menu_item && (isset($menu_item['page_arguments'][0]))) {

Thanks!

it appears you are still working in the master branch so you'll need to change that. Also I found a small typo in your .module file right now you have " * Allow to open nodes in admonistrative overlay mode." That should be an "i" instead of an "o" in "admonistrative".

All that said, I never did get this working quite right. It seemed to install fine but no matter how I crafted the URL I could not get an overlay. Here are some example URLs I tried using, keeping in mind that my site is in a "kickstart" folder inside of MAMP.

http://localhost:8888/kickstart/#node-overlay/8
http://localhost:8888/kickstart/#node-overlay/node/8

If I just do something like http://localhost:8888/kickstart/node-overlay/8 I see the node in the right theme but it is not an overlay. I'm willing to accept that it's user error but I though I followed the directions in the README file.

On another note, it does not seem to play well with PathAuto. For example if I do http://localhost:8888/kickstart/node-overlay/8 it finds the node, again not in na overlay, but if I do http://localhost:8888/kickstart/#node-overlay/mug-display which is the same node it gives me a page not found error.

How is this different then what can be accomplished using http://drupal.org/project/overlay_paths and entering node/* as a path to use overlay on?

I agree to btopro for #5, we have a similar module here http://drupal.org/project/overlay_paths

Also please add the git command to make the reviewers life easy.

You could add - "git clone --recursive --branch master http://git.drupal.org/sandbox/yuriseki/1771028.git node_overlay" in your issue summery.

Also you are still in the master branch, please create a subversion and use that.

I agree to AngryWookie, I never did get this working right.This is working with PathAuto, but it is not an overlay.

Add access checking. For example if a node is not published, content showing for anonymous users.
node-overlay/NID

I had installed this module and i found following issues:-

1) If the node is unpublished and user don't have permission to view unpublished nodes, then it generates warnings and error. Please check the attached snapshot of errors.

2) If you use following pattern in path "node-overlay/string" (string can be any thing), then it works fine. But if you use following path "node-overlay/string1/string2" then it fails.

Also, you are using master branch. Please work in version specific branch.

@angrywookie and @ramsonkr
this module is to be used from within an href link, just like this:
<a href="#overlay=node-overlay/about">about in overlay</a>

As far as I know you can't see a page using overlay by directly calling a path to the page. Overlay must be overlayed to the page you are in. So, this module works as expected.

Of course the master branch needs to be changed.

@yuriseki look here for information: http://drupal.org/empty-git-master on how to create a version branch and empty the master branch.
Also you might want to check out overlay_paths and tell us what is diferrent in your approach and communicate with the maintainers there to work together into this.
Drupal calls this collaboration than competition (http://drupal.org/node/23789)

@bserem

I agree within an href link. But in

function node_overlay_page($var) {...

there is no page access and path pattern check.

You are absolutely right about that of course

Project applications are tasks.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Added informations about my reviews of other projects