Supersized JQuery plugin

Hi,

Great job.

Please fix as many issues as you can :
http://ventral.org/pareview/httpgitdrupalorgsandboxahlofan1836214git

Manual review :
supersized.module
In theme_supersized_control_bar function, maybe you can use a template rather than using a theme function and use l() function to generate a link markup.

In supersized_page_alter function, try something like this to retrieve node full view mode :

$node = menu_get_object();
if (isset($node->nid)) {
  //ok this is a node page and you don't even have to reload the node
} 

Supersized Jquery plugin has to be considered as a library, so what about installing it in sites/all/libraraies instead of you own module?

Thanks,

Hey,

Seems like a great module, It's great you are integrating this for better compatibality with Drupal. It's something i'd definitely use.

First of all there are quite a few issues found here:
http://ventral.org/pareview/httpgitdrupalorgsandboxahlofan1836214git

These are mainly to do with naming conventions.

Another thing i thought would be worth doing for the sake of being tidy and maintaining a structure to put the supersized_context module in a sub-directory.

Another suggestion would be to put the settings forms and other functions in the module in include files so they are only loaded when needed where the function contains a lot of code.

Where you'd have something like:

function supersized_run_supersized($node, &$page) {
  module_load_include('inc', 'supersized', 'supersized.run');
  supersized_run($node, &$page);
}

in the module file.

Then all the code in the function in the supersized_run function in the supersized.run.inc file.

This probably biases more towards a tidier module structure as well.

Hi,

"This is a line that contain a long URL. I guess I should break it into another line right?"
please use url shortness service (like bit.ly). it will help you to clear warning.

Also try to clear all other errors and warnings
http://ventral.org/pareview/httpgitdrupalorgsandboxahlofan1836214git

Hello

and no that's for a reviewer to do.

The issues to do with the naming conventions in the context plugin is indeed the naming convention used by context and lots of successful modules that contain context plugins. As well as this spaces uses the same convention in declaring it's classes.

So that issue isn't so much of an issue.

Manual Review:

- Firstly there is an unneeded declaration of the version in Supersized context module, this is added automatically.

- It is unnecessary to declare file and image as dependencies in the Supersized info file as they are a part of core and are unlikely to be disabled.

- the supersized_node_settings_form function contains a lot of code, I would suggest sticking it in an include file and breaking it down. For good practice and clarity I would suggest having a structure such as this:

-- supersized
---- css
------ supersized-default.css
---- js
------ supersized.js
---- supersized_context
------ supersized_context.module
------ supersized_context.info
------ supersized_context_condition.inc
------ supersized_context_reaction.inc
---- includes
------ supersized.field.inc
------ supersized.run.inc
------ supersized_form.inc
---- templates
------ supersized_control_bar.tpl.php
---- README.txt
---- supersized.info
---- supersized.module
---- supersized.install

- In supersized_context_reaction.inc there is a miss spelling of Supersized in the file doc block. (Suersized)

- within Javascript itself is it not best to use lowerCamel for the function opening the file?

All these things are not major problems. So in my eyes it's RTBC, I suggest you get on the review bonus system though to help with reviewing.

Make sure its the git clone for public. not the private one.

A Git Administrator will give you the permissions for creating full projects and to promote sandbox projects once they have reviewed a RTBC and found no major problems.

manual review:

1. "require_once drupal_get_path('module', 'supersized') . '/supersized.field.inc';": you are including files from your own module which you already know where they are. Use something like require_once 'mymodule.inc';
2. supersized_image_default_styles(): shouldn't all text strings in there run through t() for translation?
3. "module_load_include('inc', 'supersized', 'supersized.run');": no need to use module_load_include as you are including files from your own module which you already know where they are. Use something like require_once 'mymodule.inc';
4. supersized.module: does your module only work with nodes? Please add that restriction to the README.txt if it applies.
5. "$plugin = context_get_plugin('reaction', 'am_billboard')": what is am_billboard? Please add a comment.
6. "3 => 'Slide Right',": all user facing text must run through t() for translation.

Although you should fix those issues they are not application blockers, so ...

Thanks for your contribution, ahlofan!

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.