[aur-dev] Adding the HE language.

[Lukas Fleischer]

On Mon, Feb 14, 2011 at 09:58:50PM +0100, PyroPeter wrote:
> >From 541c18f86d970051d837e9dd75e1122292d1fd4f Mon Sep 17 00:00:00 2001
> From: PyroPeter <abi1789 at 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 at 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
>