[D7] Views COntent SCroll from EVerywhere

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /var/www/drupal-7-pareview/pareview_temp/views_coscev.test
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   90 | WARNING | Unused variable $node.
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

FILE: /var/www/drupal-7-pareview/pareview_temp/views_coscev.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 7 | ERROR | It's only necessary to declare files[] if they declare a class or
   |       | interface.
--------------------------------------------------------------------------------

if you have define hook_views_api.them remove

files[] = views_coscev.views.inc

http://drupalcode.org/sandbox/stred/2123119.git/blob/HEAD:/views_coscev....

Hi stred,

Initial Impressions

Whaaat? Haha, chaotic, I like it. I think there might be use for a module like this.

Administration

Works as expected, got it up and running without reading any documentation, setup was intuitive. Administrative texts could use some polishing but overall good.

Browser compability

Tested and working in FF 25, Chrome, IE11

Code

Coding standards are followed thorughly.

You're missing a reference operator on line 10 of views_coscev_style_plugin.inc
Strict warning: Declaration of views_coscev_style_plugin::options_validate() should be compatible with views_plugin_style::options_validate(&$form, &$form_state)

You should review your function block comments, some of them are incorrect.
views_coscev.module(#24) should be hook_coscev_views_pre_view for example.

I do not understand the use of $GLOBALS['views_coscev_displays'], can you elaborate on that?

Other

I think you need to specify the theming part on the project page as it limits the scope
of the module to a large extent.

All in all - nice module, but needs some work!

Hey Stred,

Good module but needs a little work, i was looking for something like this since a few days, hope to see this soon as a full project, Effects are working nicely.

My Reviews :

1. I got a strict warning after creating a view and this warning is on the page also. what i understood this warning is coming from the function options_validate parameters. (https://api.drupal.org/api/views/includes%21plugins.inc/function/views_p...),

public function options_validate($form, &$form_state) {

here replace $form with &$form solves the problem, (correct me if i am wrong.).

2. Coder module gives some warning for incorrect coding standards.

3. In the view, under format settings, as i understood the title settings are for view title, and these settings seems to have some problem with them, they didnt worked for me.

else everything looks good from user perspective.

Overall Awesome work and a nice module.

Hello again,

The reference operator is, as gaurav.goyal pointed out, the "&" in front of $form.

Another suggestion would be to replace $GLOBALS['views_coscev_displays'] with a static variable instead, since you seem to reset it each page call anyway. That would remove the need of hook_init. Also, unless I'm mistaken, can't you increment the variable in the process function instead, or should it be incremented for each view, even those not associated with this module? If that's possible, you can remove hook_views_pre_view as well.

Ah, I see why a static won't work in the process function. Sorry for the confusion. How about removing hook_init() and using a static in hook_views_pre_view() instead?

function views_coscev_views_pre_view(&$view) {
  $display_count = &drupal_static('views_coscev_displays');
  $display_count++;
}

Does the process function implement hook_process_HOOK? Block comment says hook_process, but that doesn't seem right.

I would also like to see a condition in hook_views_pre_view() that the view in question actually uses this module (otherwise there will be problems if and when the requirement of a blank page is resolved). I checked the data provided to the hook and something along the lines of this might work;

function views_coscev_views_pre_view(&$view) {
  if(isset($view->plugin_name) && $view->plugin_name == 'views_coscev'){
    $additional_display_count = &drupal_static('views_coscev_displays');
    $additional_display_count++;
  }
}

Then check if &drupal_static('views_coscev_displays') is greater than zero in your process function, as plugin_name wont be set in the page display. I cannot, however, guarantee that this is sound, but it might give you some ideas.

I would have someone more familiar with Views API have a second look before changing status to RTBC, as my limited knowledge of it prevents me from doing it with a clear conscience. Consider getting a review bonus by reviewing other applications (shameless plug) to lure the experts out of their lairs.

Also, providing database dumps of your examples would make it easier to test the functionality.

I'll leave status as "Needs review" as I don't think any of my blabberings are blocking issues.

Hey Stred,

#2. On reviewing with coder it gaves error in your .module file, attached is the screenshot. There should only a single space before and after any operator (https://drupal.org/coding-standards#operators), In Line no. 50-55 of .module file, there are more then one spaces with "." operator.

 $onpage_css = $display_class . ' .top-hor-left, ' . $display_class .  ' .top-hor-right {top: ' .  _views_coscev_px_or_percent($options['margins']['margin_top']) . ';}
   ' . $display_class . ' .bottom-hor-right, ' . $display_class .   '  .bottom-hor-left {bottom:  ' .   _views_coscev_px_or_percent($options['margins']['margin_right']) . '}
   ' . $display_class . ' .top-vert-left, ' . $display_class .   '  .bottom-vert-left {left:  ' .   _views_coscev_px_or_percent($options['margins']['margin_left']) . ';}
   ' . $display_class . ' .top-vert-right, ' . $display_class .  '  .bottom-vert-right {right:  ' .  _views_coscev_px_or_percent($options['margins']['margin_right']) . ';}';

there are also some comment block missing related issue in views_coscev_style_plugin.inc, views_coscev.test, views_coscev.js files.

#3. yes, titles are working fine :).

1. Functions and methods should be named in lowerCamelCase in the views_coscev.js. Like -

function pxOrPercent(field, settings) {

2. Control statements should have one space between the control keyword and opening parenthesis, to distinguish them from function calls in the views_coscev.js. Like -

 if (setting == 'fade') {
   this.css('opacity', amount / 100);
 }

3. Variables in JavaScript should be lowerCamelCased in the views_coscev.js. Like -

var diff_scroll should be diffScroll.

Please see the javascript coding standard. https://drupal.org/node/172169

/**
 * Implements hook_process().
 */
function views_coscev_process_views_coscev_view(&$vars) {

This definitely does not implements hook_process(). The module is views_coscev, so the function-name would be views_coscev_process, not views_coscev_process_views_coscev_view().

-----------------

/**
 * Private utility function to parse a pixel or percent plugin setting .
 */
function _views_coscev_px_or_percent($val) {

- Each parameter of a function must be documented with a @param tag.
- If a function has a return value, it must be documented with a @return tag.

This should be -

/**
  * Private utility function to parse a pixel or percent plugin setting.
  *
  * @param string $val
  *   The pixel or percent value.
  *
  * @return string
  *   Parse value of pixel or percent.
  */
function _views_coscev_px_or_percent($val) {

See the API documentation and comment standards https://drupal.org/node/1354#functions

I have tested your module and its working fine. A little performance issue in your js file you are using $('body') quite a times on line 8,9,56,126,127,157,158 . Good javascript method would be

var body=$('body');
body.something();

Thanks for your contribution.

Not a lot left on this module. Some very small stuff and this can get marked RTBC. Really only the version in the .info file and whitespace around == is required. But the rest of my comments are good info and should be followed-up on.

.info file is missing version details:
version = 7.x-1.x

css file:
0% for top, bottom, etc is redundant.

Whitespace around the equals:

function views_coscev_views_pre_view(&$view) {
  if ($view->display_handler->default_display->display->display_options['style_plugin']=='views_coscev') {

}

js file:
$.fn.position_me, $.fn.title_reset, etc.
$.fn namespace is on the root. This can / maybe will causes issues with other modules. Namespace them to the module and you'll be certain not to step on other's toes.

For goodness sake, assign body to a variable and pass it 'context'.
$('body', context)

Comments in js should start with a capital letter and end with a full stop. i.e. be sentences.

tpl files:
Make it easier on themers and format the if statements with some more whitespace.

<?php if ($action_links): ?><ul class="action-links">
  <?php print render($action_links); ?></ul>
<?php endif; ?>

* Count the amount of displays used (see line 113).
A little more helpful comments would be great. Line 113 has nothing to do with counts.

Test file:
Ln 90: Avoid unused local variables such as '$node'.

stred, you are right. Normally version numbers are generated by Drupal.org package manager script. Hence its not necessary to include in .info

FYI, These numbers are generated by referring tag (or) branch name of current release.

CSS:
0% is the default.

JS:
$('body', context).hide()
See http://api.jquery.com/jQuery/#jQuery-selector-context

tpl:
I won't argue with what's in core, however its hard to read :)

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. views_coscev_process_views_coscev_view(): doc block is wrong, this is not hook_process() but rather hook_process_VIEW_ID_view() or something?
2. views_coscev_process_views_coscev_view(): where do you sanitize $title before printing to avoid XSS problems? Usually this could be a security issue, but since the "administer views" permission is required to set a Views title this can be fixed as a standard bug. See also https://drupal.org/node/28984

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to ELC as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, stred!

I updated your account so you can 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 stay 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.