[D7] CollagePlus

1. @line 15 in collageplus.module file :-
$output = ' t('

Installation

1. Download and install Collageplus and Libraries API modules.
2. Downlod CollagePlus library and extract it in libraries directory.
3. Rename extracted directory to "collageplus".

Usage

1. Create a view with image field.
2. Select "Collageplus" display format.
3. Select the image field as grouping field.
4. Set appropriate format settings for "Collageplus".
5. Clear cache.

', array('@url' => url('admin/config/system/actions')))';

This is not correct way.

For reference check it,

https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7

  // DO NOT DO THESE THINGS
  $BAD_EXTERNAL_LINK = t('Look at Drupal documentation at !handbook.', array('!handbook' => '<a href="http://drupal.org/handbooks">'. t('the Drupal Handbooks') .'</a>'));
  

// Do this instead.
  $external_link = t('Look at Drupal documentation at <a href="@drupal-handbook">the Drupal Handbooks</a>.', array('@drupal-handbook' => 'http://drupal.org/handbooks'));

2. You must check fix these errors first
http://pareview.sh/pareview/httpgitdrupalorgsandboxsushilhanwate2165711git

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
29 | WARNING | Line exceeds 80 characters; contains 86 characters
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/collageplus.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
14 | WARNING | Line exceeds 80 characters; contains 83 characters
--------------------------------------------------------------------------------

FILE: ...ww/drupal-7-pareview/pareview_temp/collageplus_plugin_style_collage.inc
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
13 | ERROR | Class name must begin with a capital letter
13 | ERROR | Class name must use UpperCamel naming without underscores
18 | ERROR | Method name
| | "collageplus_plugin_style_collage::option_definition" is not in
| | lowerCamel format, it must not contain underscores
18 | ERROR | No scope modifier specified for function "option_definition"
32 | ERROR | Method name "collageplus_plugin_style_collage::options_form" is
| | not in lowerCamel format, it must not contain underscores
32 | ERROR | No scope modifier specified for function "options_form"
106 | ERROR | No scope modifier specified for function "render"
146 | ERROR | No scope modifier specified for function "validate"
--------------------------------------------------------------------------------

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.

Nice module. I went through the manual review checklist and nothing really stood out. That said, I am very new to this process, so I may be missing something :) The automated review pointed out a spacing/indent issue on ln 36 of the .module file.

I was able to produce a view with this plugin and it works well! This may be out of scope for the review process but something a little confusing to me was *why* the grouping field is required. You point that out in README.txt and threw in an error message in the view UI, but maybe *why* it's required this could be clarified in README for anyone who's curious.

Thanks!

Hi!

So I noticed that starting on line 83 of your module file you have three functions that have very, very similar code - the _collageplus_*_libary_path() functions.

It looks like they could pretty easily be combined to take an argument (the file names you're trying to check for). You could also add an optional argument for the sub-directory ("extras")

function _collageplus_get_library_path($which_file, $sub_dir = NULL) {
  $collageplus_path = libraries_get_path('collageplus');

  if (!empty($collageplus_path)) {
    // Attempt to use minified version of jQuery collageplus plugin.
    if (file_exists("$collageplus_path/$sub_dir" . "jquery.$which_file.min.js")) { /** if sub_dir is not null, make sure you add "/" do the end of it */
      $path = "$collageplus_path/$sub_dir" . "jquery.$which_file.min.js";
    }
    // Otherwise use non-minified version if available.
    elseif (file_exists($collageplus_path/$sub_dir" . "jquery.$which_file.js")) {
      $path = "$collageplus_path/$sub_dir" . "jquery.$which_file.js";
    }
    else {
      $path = FALSE;
    }
  }
  else {
    $path = FALSE;
  }

  return $path;
}

You lose a little bit as far as readability goes, but I feel it's better not to have basically duplicated code... as well as a function that is a bit more robust with the potential for future use.

> description = Drupal Module to integrate CollagePlus jQuery plugin.

No need to say 'Drupal module'. The reader is looking at this on the module admin page. They already know it's Drupal, and a module ;)

> package = Collageplus

Don't create a new package name if you only have one module to put in it.

> function collageplus_requirements($phase) {

Doesn't Libraries API take care of this?

$output = t('<h2>Installation</h2>

It's not usual to put installation instructions in hook_help!() It's shown to users who have already installed the module!

> function _collageplus_library_path() {

Again, can Libraries API not help more here?

> $form['grouping']['#description'] = t('Specify image field.');

Seems really odd to use the grouping setting to specify the image field. Is there something in the data handling in the view that requires this?

      <div class="Image_Wrapper">

Drupal CSS classes by convention are lower case with hyphens: image-wrapper. You should also prefix with your module's name.

> if (module_exists('libraries')) {

You've declared libraries as a dependency: you can trust that it's there.

I must say, interesting module with clear description of what it does and steps to implement this. Though, below are the few things I found in addition to the joachim's comment

1. Some Parview issues as mentioned by Rkumar, please visit http://pareview.sh/node/557276/repeat.
2. On the Help Page the Download link to Collage Plus JS library is http://collageplus.edlea.co/ which I think should be http://collageplus.edlea.com/
3. I'm not too sure if the thing I am expecting here is right or not but if someone somehow miss to download the JS library or to rename it the mentioned name, a warning / error should be displayed on the created view. Currently no data is displayed instead. Need some error handling there.

Mandar

Hi, thank you for your module, I already using it on client's web, but I found few bugs:

collageplus.js

$(window).load(function(){
$(document).ready(function(){

Firstly this code is not needed because we know dimension of images via http://collageplus.edlea.com case 2 and secondly it doesn't work properly if using AJAX

collageplus.theme.inc

drupal_add_css($library_path . '/support/examples.css');

Please do not add css styling if it's not necessary and breaks site styling

I see that no comments provided whether last issues resolved or not. Also there are open bug issues in sandbox issue queue. So changing back to 'needs work'.

P.S.: @sushyl, have you contacted Collage Formatter module's maintainers with proposition of support on adding new features your sandbox module provide?

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Then you definitely should request a maintainership over an abandoned project.

Looks like a nice module.
Please consider implementing hook_requirements() in your module.install file. This is an example of its implementation in field_slideshow module :

if ($phase == 'runtime') {
    $t = get_t();

    $path = libraries_get_path('jquery.cycle');
    if ($path == '') $path = '/sites/all/libraries/jquery.cycle';
    if (file_exists($path . '/jquery.cycle.all.min.js') || file_exists($path . '/jquery.cycle.all.js')) {
      $requirements['field_slideshow_cycle_plugin'] = array(
        'title'     => $t('JQuery Cycle plugin'),
        'severity'  => REQUIREMENT_OK,
        'value'     => $t('Installed'),
      );
    }
    else {
      $requirements['field_slideshow_cycle_plugin'] = array(
        'title'       => $t('JQuery Cycle plugin'),
        'value'       => $t('Not found'),
        'severity'    => REQUIREMENT_ERROR,
        'description' => $t('You need to download the !name and move the downloaded js file(s) into the %path folder of your server.', array('!name' => l($t('JQuery Cycle plugin'), 'http://jquery.malsup.com/cycle/download.html'), '%path' => $path)),
      );
    }
}

@AjitS Is there something else that you could find that is blocking the review? Should this be in RTBC?

@naveenvalecha I didn't take a closer look at the project :-) It was just a suggestion.

Manual Review

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

No duplication
[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch
[Yes: Follows] the guidelines for master branch.

Licensing
[Yes: Follows] the licensing requirements.

3rd party assets/code
[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md
[Yes: Follows / No: Does not follow] the guidelines for in-project documentation and/or the README Template.

The README.txt file follows general guidelines, but I noticed the following issues in that file:

CollagePlus is misspelled in the description

The description here and in the project page is self-referential. (I know it's copied from the download page for the library.) It should tell a potential user what a "Collage image gallery" is. From the picture on the project page, it looks like a Collage image gallery is a container for images of different sizes. The images are scaled to a defined row height and arranged into responsive strips in the container. If so, just add the stuff I wrote or something similar to your description.

There are many modules that approach the problem of viewing image content from slightly different perspectives. The better they can be described, the easier it can be to determine whether or not a particular module will work for a given application.

Code long/complex enough for review
[Yes: Follows] the guidelines for project length and complexity.

Secure code
[Yes: Meets the security requirements]

Coding style & Drupal API usage

The automated review noticed a few indentation issues in the .js file.

The code looks essentially clean and reasonably well-documented, but I found several issues when I installed it to a local copy of a production site.

This module seems to be very close to filling a need that I'm sure a lot of people, myself included, have experienced. I really like having a nice, responsive way to display images of mixed sizes.

Because CollagePlus is a Views-based module, please place it into the Views package. Packages exist to group modules together by function and use, and this module belongs in that package.

I recognize that some of the behavior may be from the library. Other behavior seems to be associated with shortcuts in dealing with Views settings.

I did notice one significant area where this module "does not play well with others" that I would like to see fixed before using this module on a production site. I use the Image Link Formatter module to link an image to its associated content. I want my picture gallery to link to more information, allowing us to use the pictures as an index.

There should be a description of view settings. Text for Fade speed mentions timing of other effects. What are they?

I tried switching view settings between Horizontal and Vertical, but nothing appears to change.

Enabling the Title field as a caption doesn't access the Title field associated with the node containing the image. Note that my images are from the Library in the Media module where alternate and title text have been set up. Each node I want to place into the collage includes a single Image field and a Title field. (There are other fields, too.)

It's probably better to default the last row image scaling to false. The collection of images I'm using includes an image that's quite tall for its width. As luck would have it, that was my last image. It was placed on the last row, by itself. That image was scaled to require several times as much height as all other images together. And that option doesn't work. While I recognize that it may be a limitation in the library code, the scaling really should allow for a maximum row height to ensure that each individual image fits into its display window.

The View format includes "Show" options. The display ONLY works if the "Fields" option is selected. Even then, the actual settings appear to be ignored. Perhaps, code similar to that used for the Table format could be used to avoid display of the "Show" options.

Field settings are permitted, but are ignored except that the image probably needs to be defined as a field so it can be used for grouping.

Great! I really want to be able to use it.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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