Pretty Pagination

Welcome,

you need to set the status to "needs review" if you want to get a review. See http://drupal.org/node/532400

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

Other things to consider:

Module description in the info file should be as short and descriptive as possible. Common pattern "Changes default pagination to 'x of yy' " od somethins along the lines.

Your Readme.txt file has lines longer than 80 characters which is the suggested maximum length.

In your js file you declare ppsubmit function which is not yousing your modules namespace. Best practice is to start all functions with your modules namespace.

When I enter numbers into the input, console log in Firebug (FF) trhows an error "ReferenceError: ppnosubmit is not defined". This is because on line 172 of your module file you have onkeyup='return ppnosubmit($params,1)' and this function is not defined.

Hi zbabtkis,

This is a nice module and great work done by you.

Automated Review:
There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

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.

Source: http://ventral.org/pareview - PAReview.sh online service

Manual Review:
1) Please use proper sanitization function if you want to use query strings on line no 181 in prettypagination.module you have written
$urllink = $_GET['q'];
2) You should not generate html by youself prettypagination.module has below code on line 362 instead of usig anchor tag you should use l() function.
return '<a' . drupal_attributes($attributes) . '><i>' . check_plain($text) . '</i></a>';

Removing review bonus tag you can review other application and add review bonus tag again.

Installed and reviewed the code. I'm still new to all of this myself so I saw nothing that stuck out as bad to me in the code itself.

I did notice the sandbox project page has no description? What exactly does this change? What can be customized? What does it do? It's a stylized pager... but is that it?

Also, as a stylized pager, it's visually broken in firefox for me on a base D7 install with Bartik for the theme. I imagine this is meant as a tool to themers though? Again, describing this in the project would be helpful.

Lastly, the << and < on the first page that do nothing is a bit awkward. Not sure if it's broken in a sense but... I'm not sure I'm finding that particularly intuitive.

Hi,

Manual Review:

1) This prettypagination_pager_link() function has security issue you should not directly use $_GET['page'], use proper sanitization function.
2) I have a suggestion if you can provide a template for pager it will be helpful for users so that anyone can override it.

Hi wuinfo,

I think you forgot to change the status.

* You include your js and css file both in prettypagination.info. This way both files are loaded on every site (even those without a pager). Therefore it would be much better to load the files using drupal_add_css() and drupal_add_js() in the function <?php prettypagination_pager() ?>.
* Its best practise to use Drupal behaviors for custom javascript functions (see The Drupal Javascript API. So its better to rewrite your submit-function to "Drupal.behavior" and remove "onkeyup", "onkeydown" and such "old-fashioned" markup ...

Hi @wuinfo,

I like this functionality! I downloaded and enabled your module (using Drush), and was able to see the new pagination. The links work, however, on my particular browser, the JavaScript produced errors when I tried to use the textbox (id=prettypagination). I was on the first page, and I entered "3" in the text box, and pressed the ENTER key.

Accessing my browser's console showed:

event.layerX and event.layerY are broken and deprecated in WebKit. They will be removed from the engine in the near future.
prettypagination.js:23
Uncaught TypeError: Object http://dgd7.dev/node?page=2 has no method 'click'
event.layerX and event.layerY are broken and deprecated in WebKit. They will be removed from the engine in the near future.
prettypagination.js:23
Uncaught TypeError: Object http://dgd7.dev/node?page=2 has no method 'click'

See attached screen shot.

My browser is running in Ubuntu, and in my web server logs, I see this for my USER_AGENT:

"Mozilla/5.0 (X11; Linux i686) AppleWebKit/535.19 (KHTML, like Gecko) Ubuntu/11.10 Chromium/18.0.1025.168 Chrome/18.0.1025.168 Safari/535.19"

This would be a blocking issue for me, but I can appreciate it would take some time getting an Ubuntu environment up and running. If this issue is difficult to get past, then perhaps it can be called out in the README.

Good luck!

Hi @wuinfo

I confirmed that the fix you made now works for my environment! (I did a 'git pull' in the prettypagination directory, then reloaded my home page.)

I'm not experienced enough to mark this issue as "RTBC" (Reviewed & Tested by the Community), but I feel it's very close. Good luck with the rest of the process. I like this module for sure, and look forward to seeing it become a full project.

Hi.

I like the module. The site I tested it with makes intensive use of SOLR as "navigation" with 10000+ nodes.
So, this would probably be a typical use case.

1)
Installation went well and works as described, did not try layouting it, though.

2)
It does not work with SOLR.
On the SOLR search result page the link looks like http://mysite.com/search/site/searchterm?page=1 => pagination works
When adding a facet to the search the link looks like http://mysite.com/search/site/searchterm?f[0]=im_field_type%3A2011
Using the arrows works but when entering a page numberand pressing return the URL changes to http://mysite.com/search/site/searchterm?page=13 and all selections are lost.

Hope you can fix this quickly because it looks really useful.

Manual review

** all files are executable

A CSS-stylesheet should not be executable as a program.

You must remove execution permission from all files in this module. On Linux, you can do this by running chmod a-x *. I have no idea how you do this in Windows.

** prettypagination.module, line 147

of is not translatable.

This module is not fully functional with disabled JavaScript. For example, page cannot be changed by inputting a new page number into the textfield. User is forced to navigate using prev/next links. Other people may disagree, but for me it a very serious issue. Do you plan to fix it?

Besides, your JavaScript code does not check what has been inputed into that textfield. Try typing 'xyz' and hit enter: you will be redirected to page NaN.

Overall, I am not happy to see that theming hooks defined in this module are 90% identical to Drupal pager's default hooks.

Here is a proposal. Instead of duplicating pager's functionality and markup, do not override theming hooks and let Drupal generate the default pager. And then you can completely override pager's markup with you JavaScript on the client-side. I see three advantages of this approach:

1. No code duplication. Less code to maintain;
2. Avoidance of possible conflicts with other modules that change pager's functionality. For example, your module breaks Views' AJAX feature;
3. Fallback for users with disabled JS (e.g. myself). They will simply use the default pager.

It is possible to generate perfectly themable markup with JavaScript, and if you provide a detailed theming guide with your module, other developers will know what to do.

Edit: spelling.

Hey there! I took a gander through what you have written (again), and installed it on a local D7 install. Still working, I reviewed your code and obviously it's come a really long way. I feel your pain, mine is going similiar!

Anyways, good work. I have no issues I can spot... but I'm a noob :)

Hi,

• tested it once again with SOLR and the reported bug with loosing parameters seems to be fixed now.
• when a search result has 35 pages and you enter 36 you see nothing. Maybe you could jump to the last page instead?
• same when entering -1 (actually gets converted to -2)
• prettypagination.module line 186: I can't see why you hardcode the div. Wouldn't it be an option to move it into the theme as well?

regards
hhhc

Wow, thanks for fixing everything, wuinfo.

Few minor things:

1. Coder sniffer found one error in your code;
2. prettypagination.module, line 22 You should remove a period from the end of the sentence. That way it will look more consistent with labels of other fields in this form;
3. Sandbox project page still does not have a description;
4. prettypagination.module, lines 47-48 Use check_plain() to escape values of variables before inserting them into HTML. This will protect code from XSS (is there any guaranty that these variables will contain only integers under any circumstances?);

I think, these are not major issues, and for me this module is clearly RTBC.

manual review:

1. "theme_get_setting('toggle_prettypagination')": theme settings should be prefixed with your module's name to avoid name collisions with others.
2. prettypagination_preprocess_pager(): do not concatenate variables to t() calls, use placeholders with t() instead.
3. prettypagination_preprocess_pager(): this is a preprocess function and should not contain any markup. Either you replace theme_pager() or you add your own theme function that assembles the markup. Themes should be able to override this.
4. "if (function_exists("overlay_get_mode")) {": why do you need that? Please add a comment.
5. prettypagination_form_system_theme_settings_alter(): why do you check for $form_state['build_info']['args']? Please add a comment.

Although you should definitively fix those issues they are not critical application blockers, so ...

Thanks for your contribution, wuinfo!

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:

• 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 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.

Just leave it at fixed, the issue will close automatically in two weeks.