Posted by AmbikaFR on September 25, 2012 at 2:24pm
12 followers
Jump to:
| Project: | Drupal.org Project applications |
| Component: | module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (won't fix) |
Issue Summary
The Views Reveal module is a Views Style Plugin that can be used to output a view in a Reveal presentation. This module uses the JS plugin Reveal, made by Hakim El Hattab.
Reveal integrates very nice HTML5 presentations, like Power Point used to do.
This module will create a View Reveal type that will display nodes in a Reveal presentation. Settings are available for keyboard controls, theme, transitions effects, and more. With grouping rows, you can create vertical views or horizontal views.
Links
Project : http://drupal.org/sandbox/ambika/1794520
Git : git.drupal.org:sandbox/ambika/1794520.git
This module is used for Drupal 7.
Comments
#1
Welcome,
please get a review bonus first. Then try to fix issues raised by automated review tools: http://ventral.org/pareview/httpgitdrupalorgsandboxambika1794520git
#2
It seems that your account isn't personal. This is not allowed according the terms (you can read this notice on the registration page)
#3
Ok, sorry about that. Can i close this issue and recreate one with my personnal account?
#4
You can just change your username and all details on your profile: http://drupal.org/user/2309448/edit
#5
Ok, thanks for your answer. I ve just change my profile.
#6
@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.
#7
Ambika and AmbikaFR are different companies. Ambika is an indian company, and AmbikaFR is a french company. I work in the french Ambika company, and AmbikaFR is now my own account, as a Ambika Drupal developer.
#8
Here are my 3 reviews of "needs review" projects :
http://drupal.org/node/1637712#comment-6524558
http://drupal.org/node/1267234#comment-6528350
http://drupal.org/node/1722030#comment-6524250
Regards,
#9
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
#10
Ok klausi, and thanks for your help. i am looking for code style issues, reported by PHP Sniffer. 2 questions :
- 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 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? (In fact, it's impossible to extend views core plugins without underscores in function names)
Regards,
#11
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 would say, yes: dawehner noted recently "we can't change the (Views-) API just to satisfy the code style guidelines" ;)
#12
The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
#13
Ok Klausi, i am going to work with libraries to include Reveal.js. Thanks for your precisions.
Regards,
#14
Adding PAReview: review bonus tag.
#15
I have just push the last revision of Views Reveal :
Thanks in advance for your review,
Regards,
AmbikaFR.
#16
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.
<?php/**
* 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?
<?php/**
* 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?
<?php/**
* 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.
<?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)
<?php'#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.
<?php$sets[$group_content]['is_vertical'] = (empty($group_index)) ?
FALSE : TRUE;
?>
<?php$sets[$group_content]['is_vertical'] = !empty($group_index);
?>
This is the first overview of the code. Maybe I will add something later.
#17
Hi Berdart,
At first, thanks for your manual review and sorry for the very long delay for my answer.
Yes, i need to implement this hook because reveal display is not aware of page variables if this hook is not implemented (nothing is passed to the template).
Do you mean a theme function ? I'am not sure it's the best place for this kind of code. The code just add css and js ressources in case of Reveal views displaying, it doesn't render HTML code.
I have just edit my code according your other suggestions. I have also move views and templates files in specific folders. I have also moved the project in a 7.x-1.x branch.
Regards,
AmbikaFR.
#18
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 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?
#19
Hi Berdart,
Sorry for the very long delay again !
I am not sure it's really possible, beacause i have to suggest a page template to Drupal (not a views template), in order to display Views Reveal presentation in suitable way.
In fact, the code checks if the page is "views page" with views_get_page_view() function. After that, the code checks if the current view is a Reveal View.
I have push the code on a 7.x-1.x branch. Here is a link to the last commit.
#20
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.
#21
Hi Jordan_Fei,
Reveal settings are defined by Views options_form, in views_plugin_style_reveal Class. These options are populated by default by Views, with #default_value settings. Each #default_value is defined, in options_form. So, backend check that each Reveal settings are certainly define. In this configuration, i don't think that a JS check is necessary.
#22
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:
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:
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...
#23
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.