On Mon, Feb 14, 2011 at 09:58:50PM +0100, PyroPeter wrote:
From 541c18f86d970051d837e9dd75e1122292d1fd4f Mon Sep 17 00:00:00 2001 From: PyroPeter <abi1789@googlemail.com> Date: Mon, 14 Feb 2011 20:58:42 +0100 Subject: [PATCH] pkg_search_results: rewrite of pagination
* Most of the PHP-code was moved to pkgfuncs.php to keep the template simple.
Signed-off-by: PyroPeter <abi1789@googlemail.com> --- web/html/css/arch.css | 17 +++++--- web/lib/pkgfuncs.inc | 28 +++++++++++++ web/template/pkg_search_results.php | 77 ++++++---------------------------- 3 files changed, 53 insertions(+), 69 deletions(-)
diff --git a/web/html/css/arch.css b/web/html/css/arch.css index c3ed3aa..203d9da 100644 --- a/web/html/css/arch.css +++ b/web/html/css/arch.css @@ -328,19 +328,24 @@ blockquote.code { text-decoration: none; }
-#pages { margin: 5px; } -#pages .page_num { +.pageNav { + margin:5px 0; +} +.pageNav .page_num { border: 1px solid #ddd; padding: 2px; color: #0771a6; }
Why don't you stick to the coding style and use lowerCamelCase instead of underscore_separated_class_names? Using non-standard capitalization doesn't contribute to the consistency of our code base.
- -#pages .page_num:hover { +.pageNav * { + display:inline-block; + text-align:center; + min-width:3ex; +} +.pageNav .page_num:hover { border: 1px solid #8faecd; color: #333; } - -#pages #page_sel { +.pageNav .page_sel { border: 1px solid #8faecd; padding: 2px; color: #333;
Same here. And again, you should have split this patch into two separate ones. There's no relation between CSS adjustments and the separation of action and view. Keep your patches small.
diff --git a/web/lib/pkgfuncs.inc b/web/lib/pkgfuncs.inc index 2f69321..0420054 100644 --- a/web/lib/pkgfuncs.inc +++ b/web/lib/pkgfuncs.inc @@ -559,6 +559,34 @@ function pkg_search_page($SID="") {
if ($total > 1 || $total == 0) { + # calculation of pagination links + $per_page = ($_GET['PP'] > 0) ? $_GET['PP'] : 25; + $current = ceil($first / $per_page); + $pages = ceil($total / $per_page); + $templ_pages = array(); + + if ($current - 1 > 1) + $templ_pages[__('First')] = 0; + + if ($current > 1) + $templ_pages[__('Previous')] = ($current - 2) * $per_page;
I'd just use the same criteria for both "First" and "Previous" buttons. It might be right that having separate "First" and "Previous" links on the second page doesn't make a lot of sense but those constantly appearing and disappering pagination links in search results are really confusing when actually browsing stuff imho. I'm open to different opinions tho.
+ + if ($current - 5 > 1) + $templ_pages[] = "ellipsis";
I'd prefer something like "$templ_pages['...'] = false;" here so we can just use "if ($pagestart === false) echo $pagenr;" to display text without ".page_sel" wrapper spans and don't need to deal with strange special cases later.
+ for ($i = max($current - 5, 1) ; $i <= min($pages, $current + 5) ; $i++) {
Please don't put spaces before those semicolons.
+ $templ_pages[$i] = ($i - 1) * $per_page; + } + + if ($current + 5 < $pages) + $templ_pages[] = "ellipsis";
Same as above.
+ + if ($current < $pages) + $templ_pages[__('Next')] = $current * $per_page; + + if ($current + 1 < $pages) + $templ_pages[__('Last')] = ($pages - 1) * $per_page;
Same as with "First" and "Previous".
+ include('pkg_search_form.php'); include('pkg_search_results.php'); } diff --git a/web/template/pkg_search_results.php b/web/template/pkg_search_results.php index 4830ca8..57d574c 100644 --- a/web/template/pkg_search_results.php +++ b/web/template/pkg_search_results.php @@ -113,73 +113,24 @@ for ($i = 0; $row = mysql_fetch_assoc($result); $i++) { </td>
<td align='right'> - <span class='f4'><span class='blue'> - <?php print __("Showing results %s - %s of %s", $first, $last, $total) ?> - </span></span> [...] - <a class="page_num" href='packages.php?<?php print mkurl('O=' . ($total - $_GET['PP'])) ?>'><?php echo __('Last') ?></a> - <?php endif; ?> - + <div class="pageStats f4 blue">
Where did you define ".pageStats"? You should use underscores as word separators here as well.
+ <?php print __("Showing results %s - %s of %s", $first, $last, $total) ?> + </div> + <div class="pageNav">
Use underscores, see above.
+ <?php foreach($templ_pages as $pagenr => $pagestart) { ?> + <?php if ($pagestart === "ellipsis") { ?> + ...
As mentioned above, I'd just use "if ($pagestart === false) echo $pagenr;" here and set page number offsets in "$templ_pages" to "false" if an entry doesn't link to a specific page range. That is a more generic solution.
+ <?php } else if ($pagestart + 1 == $first) { ?> + <span class="page_sel"><?php echo $pagenr ?></span> + <?php } else { ?> + <a class="page_num" href="packages.php?<?php print mkurl('O=' . ( $pagestart)) ?>"><?php echo $pagenr ?></a> + <?php } ?> + <?php } ?> </div> - </td> </tr>
-<?php - } -} -?> +<?php } ?> </table> </div> </form> -- 1.7.4.1