[D7] Slidesjs Slider

Automatic Review

I'll be great if you include a link to pareview:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpitabas2308935git-7x-1x

It says you must define the default branch. Here's the documentation: https://www.drupal.org/node/1659588

Also, it has some coding standard problems on the automatic validation.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes I couldn't find any contributed module that integrates the Slidesjs Jquery puggin

Master Branch

No: It does not have a default branch defined.

Licensing

No: the project has it's own LICENSE.txt file instead of expect drupal adds it (more information here https://www.drupal.org/node/1587704 )

3rd party code

No: The library slides is included into the respository and it's under the apache license http://www.apache.org/licenses/LICENSE-2.0. IMHO it's much better use it as a external library and explain into the README.txt file how to install and use it.

README.txt

Yes: The README.txt is very clean and exaplains everything ok.

Code long/complex enough for review

Yes: a bunch of lines and functions.

Secure code

Yes. : I checked al SQL querys and all of then use de views API. Didn't found any cross site, scripting possibility.

Aditional comments

on file slidesdjs_slider.module line 10 it seems there's an incomplete comment:

 * This help w

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

@Pitabas, thanks for your contribution.

Your project page contains so small information, do extend it little bit more.

Automatic Review
http://pareview.sh/pareview/httpgitdrupalorgsandboxpitabas2308935git reported number of issues. See attachment.

Also I ran coder review on your module that shows following notices:

<strong>slidesjs_slider.module</strong>

Line 41: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
          $output .= '<p>'. t('The following are demonstrations provide good examples for how to used directly your theme.').'</p>';

Line 65: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
         $output .= '<p>' . t('slidesjsSlider support different type of <a href="http://www.slidesjs.com/#docs" target="_blank">configuration options.</a> ').'</p>';

Line 65: The $string argument to t() should not begin or end with a space. (Drupal Docs) [i18n_11]
          $output .= '<p>' . t('slidesjsSlider support different type of <a href="http://www.slidesjs.com/#docs" target="_blank">configuration options.</a> ').'</p>';

Line 219: else statements should begin on a new line [style_else_spacing]

      } else {

Line 247: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
        $attachments['css'][] = drupal_get_path('module', 'slidesjs_slider') .'/assets/css/example.css';

<strong>slidesjs_slider.install</strong>
Line 16: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]
      if($phase == 'runtime') {

Manual Review
1. In .module file, wrong comment is there Implements hook_foo()., fix it.
2. In slidesjs_slider_add() function, $attachments variable is used in return statement but not declared outside any if statement, as it should be.
3. In .install file, wrong comment is there hook_schema()., fix it.
4. In slides.load.js file, personally I would recommend you to use drupal behaviors.

Though I did not check your module functionality yet so not moving to Need Work, most probably I will test this in next week.

Thanks Again :)

Hi, there
I just installed your module but, I can't get it work.
Live preview doesn't work and generated page from views is blank. I missed something ?
I think you have to implement Jquery update dependency too, to make it work.
Other observation: in Readme.txt the line 43 and the line 49 are in contradiction because zip file downloaded from http://slidesjs.com/ has different structure like folder 'examples' and folder 'sources' and if you just renamed it you don't have that path described on line 50.
And using drupal behavioursin slides.load.js it's also my recommendation.
Good luck!

Hey dude,

When copying code from http://cgit.drupalcode.org/jcarousel/tree/jcarousel.module, be aware that it might not be applicable to your module. Reading others code is a great way to get an understanding of how it all works however it can also impede your learning process & lead to poor decisions in the future.

Why are you using the Views 2 API? Shouldn't this be version 3?

/**
 * Implements hook_views_api().
 */
function slidesjs_slider_views_api() {
  return array(
    'api' => 2.0,
    'path' => drupal_get_path('module', 'slidesjs_slider') . '/includes',
  );
}

If your adding variables please uninstall them in your module.install file via hook_uninstall.

Hmmm I don't think you can just copy paste large sections of other modules and expect it to be valid. This process is about learning to maintain and develop responsibly.

Anyway lets keep going.

Why are you including a menu callback?

/**
 * Implements hook_menu().
 */
function slidesjs_slider_menu() {
  $items['slidesjs_slider/ajax/views'] = array(
    'page callback' => 'slidesjs_slider_views_ajax',
    'access callback' => TRUE,
    'file' => 'includes/slidesjs_slider.views.inc',
    'type' => MENU_CALLBACK,
  );
  return $items;
}

/**
 * Implements hook_init().
 */
function slidesjs_slider_init() {
  drupal_add_js(array('slidesjs_slider' => array('ajaxPath' => url('slidesjs_slider/ajax/views'))), 'setting');
}

The menu callback being used points to a function which doesn't exist in that file. I think you need to get a better understanding of Drupal's API by coding a module from bare bones examples.

Hi,

Thanks for the contribution, actually i am really interested in using this function is some of my works. I would spend time testing and playing around it with and also need to look deeper into the code. Mean while some small suggestions if you like to update.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
Yes
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt/README.md
Yes
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
YES
Coding style & Drupal API usage

1) Wrong comment
/**
* Implements hook_schema().
*/
function slidesjs_slider_requirements($phase) {

2) No need to include .module file in .info file
files[] = slidesjs_slider.module

3) core = "7.x" defined twice

4) Not really sure if we need slidesjs_slider_library_alter since you can add that directly in the info declaration

5) In slidesjs_slider_library unused variable $module_path

6) When we have already declared using hook_libraries_info not sure if we need hook_library

5) I think it is better to load slides.load.js,/assets/css/example.css in hook_library

6) instead of using theme_slidesjs_slider and rendering divs from function, i think it is better to create a template and preprocess data and pass it onto template file.

7) In slidesjs_slider_views_add using drupal_get_query_parameters instead of $_GET

8) In options_form please use isset whereever u r trying to access elements from options for ex:$this->options['default_css']

Prajwal

Multiple Applications

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

Project 1: https://www.drupal.org/node/2338365

Project 2: https://www.drupal.org/node/2308975

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.

I'm a robot and this is an automated message from Project Applications Scraper.