Reveal Views

Welcome,

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

It seems that your account isn't personal. This is not allowed according the terms (you can read this notice on the registration page)

You can just change your username and all details on your profile: http://drupal.org/user/2309448/edit

@klausi, I don't sure, but it seems that this user has two accounts: his own (Ambika) and another account for his company (AmbikaFR). and this account is of company. So I don'i sure, if changing profile details is a good way.

Do not forget to set your application to "needs review" once you are ready. See http://drupal.org/node/532400
Do not forget to add the "PAReview: review bonus" tag, otherwise you won't show up on my high priority list. See http://drupal.org/node/1410826

I include an external plugin "Reveal.js", is the js and css code has really to respect drupal standard coding? Much of issues are in main.css from this external plugin. Anyway, the corrections will be removed if i upgrade the plugin.

I'm not sure the plugin is licensed under GPLv2 or later. If it is not licensed this way you can't include it into your module (see Licensing FAQ).

I declare a views plugin, wich involved lowerCamel issues, because views by convention, named his function and plugins with underscores. Could we also ignore this issues?

I would say, yes: dawehner noted recently "we can't change the (Views-) API just to satisfy the code style guidelines" ;)

Licensing issues

Reveal.js appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

First of all you need to move your code into version specific branch. you can read about this here.

It's the best way to create separate directory for views API implementation called 'views' and move there all files related to the views handling.

/**
 * Add page display variables to Reveal display template.
 *
 * @param Array $variables
 *   Variables array passed to the template file.
 */
function views_reveal_preprocess_views_view_reveal_display(&$variables) {
  template_preprocess_views_view($variables);
}

Does this hook is really should be implemented?

/**
 * Preprocess the views Reveal pages to add a page suggestion.
 *
 * @see page--views-reveal-view.tpl.php
 *
 * @param Array $variables
 *   Variables array passed to the template file.
 */
function views_reveal_preprocess_page(&$variables) {
  $current_view = views_get_page_view();
  if (!empty($current_view)) {
    if ($current_view->style_plugin->plugin_name == 'reveal') {
      $variables['theme_hook_suggestions'][] = 'page__views_reveal_view';
      $style_options = $current_view->style_options;
      views_reveal_add_ressources($style_options);
    }
  }
}

Could this code be moved into style render function?

/**
 * Get the themes list for Reveal Views module.
 *
 * @return Array
 *   Each theme is an array, keyed by the machine name theme.
 *   Keys :
 *     'name' : the displayed name of the theme, in Views UI
 *     'css' : the CSS path of the theme
 */
function views_reveal_themes_list() {
  return module_invoke_all('reveal_theme_info');
}

The style of PhpDoc doesn't correspond to the Drupal code style standards. Please, read this recommendations.

$form['transition'] = array(
      '#title' => t('Transition type'),
      '#type' => 'select',
      '#options' => array(
        'default' => 'Default transition',
        'cube' => 'Cube',
        'page' => 'Page',
        'concave' => 'Concave',
        'linear(2d)' => 'Linear 2D',
      ),
      '#default_value' => $this->options['transition'],
    );

Those options also should be made translatable (use t() function)

'#default_value' => (empty($this->options['autoslide'])) ? 0 : $this->options['autoslide'],

You do not need to use extra braces around empty() function. And in many other places.

$sets[$group_content]['is_vertical'] = (empty($group_index)) ?
            FALSE : TRUE;

this can be replaced with this code:

$sets[$group_content]['is_vertical'] = !empty($group_index);

This is the first overview of the code. Maybe I will add something later.

Hi Ambika,

Regarding usage of hook_preprocess_page(). This hook is called on every page load, but your views will be displayed only on a few of them. So, IMHO, this should be moved in more specific preprocess function, for example, hook_preprocess_views() or something similar.

I have just edit my code according your other suggestions.

I dont see any changes regarding this in your code. Are you sure you pushed your changes to the repo? Or can you provide link to the commit?

In your reveal-integration.js, revealSettings object comes from backend array. So its any property you may need to check before using it. I found some you checked, but most of them you not. In order to build an robust code, you'd better check them before using them, or else javascript error would be led into.

first of all, you should make your 7.x-1.x branch the default branch, and remove your master branch.
(cloning from the default [master] branch left me confused for a while).

and, it seems you have missed some of the suggestions from previous reviews of your code, for example:

<?php
$form['transition'] = array(
      '#title' => t('Transition type'),
      '#type' => 'select',
      '#options' => array(
        'default' => 'Default transition',
        'cube' => 'Cube',
        'page' => 'Page',
        'concave' => 'Concave',
        'linear(2d)' => 'Linear 2D',
      ),
      '#default_value' => $this->options['transition'],
    );
?>

Those options also should be made translatable (use t() function)

or the use of "!empty(...)" function instead of the "? FALSE : TRUE" code.

read this answer, and follow its link (section "Documenting hook definitions") for correct documentation of custom hooks, and move your documentation on the hook implementation to its stub.
http://drupal.stackexchange.com/questions/458/how-to-create-a-hook

views_plugin_style_reveal.inc
function option_definition() has been overriden, but is simply calling its parent function. you could remove it.

  function option_definition() {
    $options = parent::option_definition();
    return $options;
  }

typos:

• "resources" has been mistyped as "ressources" in multiple locations.
• views_plugin_style_reveal.inc
  line 74: extra space in string (after "scroll.")

one last note: it seems you haven't actually added the "PAReview: review bonus" tag to your issue, and so you haven't been on the review bonus track yet. once you fix the mentioned issues, you can add the tag, and check here to make sure it has been correctly added:
http://drupal.org/project/issues/search/projectapplications?issue_tags=P...

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

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

Edit issue description.