Webform pager creates a pager on multi page webform's (using the default webform pagebreak component). The pager has the following 3 options:

  • Simple pager ('Page x of x')
  • Show percentages ('XX%')
  • Page slider with usefull tooltips

The tooltips for the 'page slider' are loaded from an extra field created during module installation called 'page description' on the pagebreak add/edit form. This is shown with jQuery tooltip when hovering a page.

I believe there a no similar projects exept maybe Webform Ajax Page which provides a Ajax next/previous pager (no page names/percentages or page slider).

Project link
http://drupal.org/sandbox/strakkie/1907226

Git
git clone http://git.drupal.org/sandbox/strakkie/1907226.git webform_pager

Drupal version

  • 7.x

Reviews

  • I have not yet reviewed any projects, i do not yet feel confident enough about my own code to review other contributers' code. Feel free to comment me about this at any time.

Reviews of other projects
http://drupal.org/node/1904782#comment-7029930 & http://drupal.org/node/1904782#comment-7032818
http://drupal.org/node/1117996#comment-7032684
http://drupal.org/node/1905876#comment-7033134

http://drupal.org/node/1917088#comment-7065390
http://drupal.org/node/1927554#comment-7109898
http://drupal.org/node/1932796#comment-7132558
Working on more ;)

Comments

Hi strakkie,

i strongly recommend you to do the review of other projects. It is an excellent way of getting self confidence, about other projects and about your own project. I have learn more doing this reviews than working on my own modules or even attending to the Drupal Sprints.

Said that, thanks a lot for your effort in contributing to Drupal.org. Let me do you some comments, so we can try to improve a bit your code.

Firstly, the link to clone your project is wrong:

git clone http://git.drupal.org/sandbox/strakkie/1907226.git webform_pager

Your code doesn't seems to follow the doxygen conventions. For example there:

/**
* Custom function for displaying page slider below webform_pager
*/
function webform_pager_show_page_slider($nid, $current_page, $total_pages) {

You should specify which variables your functions gets, what does it returns, etc... Please take a look at the official doxygen documentation: http://drupal.org/node/1354

You are having a lot of errors and issues because of that, for example because of using tabs instead double spaces, as doxygen recommends: http://ventral.org/pareview/httpgitdrupalorgsandboxstrakkie1907226git

I also think that the drupal_add_js functions should be included in the drupal hook_init, not in webform_pager_show_page_slider function.

These cvs tags are also wrong, you can remove them:

webform_pager.info:1:; $Id: $

You are also working in a wrong branch. You must work in 7.x-1.x for example, not in 7.x-1.x-dev

The most important, correct your issues in the comments, there are a lot juest because of not using the comment conventions:

http://ventral.org/pareview/httpgitdrupalorgsandboxstrakkie1907226git

Thank you very much again.

Thnx for this module.

I've installed the module and found a problem when you leave the description field empty.

<?php
Notice
: Undefined index: page_description in webform_pager_webform_component_insert() (line 239 of /Library/WebServer/Sites/Projects/drupal7/site/sites/all/modules/custom/webform_pager/webform_pager.module).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'page_description' cannot be null: INSERT INTO {webform_pager} (nid, cid, page_description) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => 10 [:db_insert_placeholder_1] => 4 [:db_insert_placeholder_2] => ) in webform_pager_webform_component_insert() (line 241 of /Library/WebServer/Sites/Projects/drupal7/site/sites/all/modules/custom/webform_pager/webform_pager.module).
?>

Also there's something not corrent in your css (see screenshot) you've set a min-heigt at the tags this can lead to different heights.

Oostie,

Thank you for your comment.
The page_description row could not contain NULL, this is fixed in the webform_pager.install

The css is fixed by using height, and i added a line-height.
Thank you again.

Issue summary:View changes

Typ error

Hello urwen,

That is a great, and awesomly fast, comment. Thank you for your time.
I will look at them and report back when i think they are fixed.

Also, thank you for pointing out that it is a good idea to review other code. I will see what i can do.

Thank you, once more.

no problem. Think that it is a "recommended practice", but i've seen projects months before being approved, and other in just a couple of hours. Just because of the reviews.

Thank you again.

Status:Needs review» Needs work

Firstly, the link to clone your project is wrong:

  • Updated topic.

I also think that the drupal_add_js functions should be included in the drupal hook_init, not in webform_pager_show_page_slider function.

  • Now using hook_init

These cvs tags are also wrong, you can remove them:

  • Removed

You are also working in a wrong branch. You must work in 7.x-1.x for example, not in 7.x-1.x-dev

  • Deleted 7.x-1.x-dev branch, now using 7.x-1.x

The most important, correct your issues in the comments, there are a lot juest because of not using the comment conventions:
http://ventral.org/pareview/httpgitdrupalorgsandboxstrakkie1907226git

  • There are no more errors on this page, yet i still doubt if i have done it completly right. I guess i need some review on this.
  • I am goint to look into a few module's code files to see if i can improve my commenting. In the meantime, let me know if you find anything.

Status:Needs work» Needs review

Issue summary:View changes

Incorrect git link, corrected

Issue summary:View changes

Added reviews

PAReview: review bonus

In your install file you should use hook_schema to create your tables instead of writing the query to do it. See http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct....

Make sure to indent using 2 spaces. If your editor allows it, set it to show whitespace so you can see where the indentation is inconsistent. When this is fixed the code will be a lot easier to for us to read and review.

Status:Needs review» Needs work

Status:Needs work» Needs review

Hi Xenophyle,

Thank you for your time. I am now using hook_schema to create my database table.

About the indenting, i have my code but could'nt really find any inconsistencies in my indenting so i moved all code back to the base line and indented my code again (perhaps the tabs to spaces function doesn't work as it should in my editor).

Thanks for you review.

webform_pager.module is not following Drupal Coding Standards.

Ventral is not giving any errors but when I run code on my localhost, I see a lot of indentation problems as pointed in comment #10

Hi a_thakur,

Thnx for your comment, you are right. I found out that my system used Windows line endings, i converted these to Unix format but git gave my a hard time about the line endings. I cannot test it on an other system but i think these errors are fixed (my Coder doesn't show the errors anymore).

Thanks in advance.

It is a nice module to have.

This module loads newer version JQuery. Can you use jquery_update module or just use the default version? Do not like a module to load 300K javascript by itself.

Hi Wuinfo,

That is indeed one of the thins i still wanted to implement. But is was having a hard time getting the default jQuery to work in my module.

I will look into it!

Status:Needs review» Needs work
Issue tags:-PAReview: review bonus
StatusFileSize
new61.11 KB

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. jquery-1.8.3.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.
  2. webform_pager_init(): why do you need hook_init()? If you want to add your JS and CSS on a node view then implement hook_node_view().
  3. webform_pager_install(): no need to set variables upon installation as you can use default values with variable_get() anyway. So I think this function can be removed.
  4. webform_pager_uninstall(): no need to drop the table yourself, Drupal will do that for you in the uninstallation process.
  5. webform_pager_schema(): please add a description to the table and the fields. See http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Status:Needs work» Needs review

README.TXT
I renamed it to .txt on my system but git didn't change it, removed and and pushed again with .txt (lowercase).

1. Fixed, only using default Drupal jquery and a custom created .js file for showing tooltips.
2. implemented hook_node_view, thanks a lot for pointing that one out. Didn't know about this one.
3. Function removed.
4. Removed.
5. Description added

Attachments, all errors fixed. Coder no longer showing any errors, except for @file block missing on my JS file, it is there? Is this normal behaviour?

Thanks a lot for your review Klausi, i will go back to reviewing other projects when i find some time later this week.

Issue summary:View changes

Extra review

Issue summary:View changes

Extra review done

Status:Needs review» Needs work
StatusFileSize
new38.14 KB

Great module! In fact, I just wrote up a blog post about it for Project Review Wednesday on the Aten Design blog. It'll be published tomorrow.

The one thing I noticed is that the page name option needs a little bit of styling love. Otherwise, looks good!

Hi cedewey,

Thats awesome! Thank your very much!

I've been meaning to change the complete style for the pager, i am not yet happy with the current styling. Will look into this, thanks again!

Gr,
Strakkie

Status:Needs work» Needs review

Fixed styling.

Back to needs review.

Try to keep the code well formatted all over the lines.
In webform_pager.module on line 15:

<?php
  $items
['admin/config/content/pager'] = array(
   
'title' => 'Webform pager settings',
   
'description' => 'Change webform pager settings',
   
'page callback' => 'drupal_get_form',
   
'page arguments' => array('webform_pager_admin_settings'),
   
'access arguments' => array('administer site configuration'),
   
'type' => MENU_NORMAL_ITEM,
   
'file' => 'webform_pager.admin.inc',
  );
?>

change to:

<?php
  $items
['admin/config/content/pager'] = array(
   
'title'            => 'Webform pager settings',
   
'description'      => 'Change webform pager settings',
   
'page callback'    => 'drupal_get_form',
   
'page arguments'   => array('webform_pager_admin_settings'),
   
'access arguments' => array('administer site configuration'),
   
'type'             => MENU_NORMAL_ITEM,
   
'file'             => 'webform_pager.admin.inc',
  );
?>

Hi likebtn,

Thank you for looking at my module. I have updated the lines.

Also, i have now separated the HTML by using two template (tpl.php) files for showing the pager.

Issue summary:View changes

Extra review

Bonus tag

Bonus tag (duplicate, woopsie)

Assigned:Unassigned» Dave Reid
Status:Needs review» Reviewed & tested by the community
Issue tags:-PAReview: review bonus

manual review:

  1. webform_pager.js: do not use $(document).ready(), use Drupal.behaviors instead. See http://drupal.org/node/756722
  2. webform_pager_show_page_slider(): no need to use check_plain() for the title since the l() function will sanitize it for you anyway. And double escaping is bad. Same for $submit_text.
  3. webform_pager_form_webform_component_edit_form_alter(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075 . Also elsewhere.

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 Dave Reid as he might have time to take a final look at this.

Klausi,

Great!! Thanks again for your review! I have fixed the points you have supplied.
Once again, thanks for your time!

I'll wait for Dave to have the final look!

Status:Reviewed & tested by the community» Fixed

Thanks for your contribution, strakkie!

I updated your account to let you 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:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Extra review