Semantic update for core pager

Your patch format is dodgy. Moreover, this is a dupe of an old issue http://drupal.org/node/68615 which, like every second patch on drupal.org, fell on its behind due to poor work-flow. Please re-roll, update or merge the two patches and I will happily review it.

Thanks.
-K

I will review this too, if you re-roll it as per Zen's suggestions. Good stuff. :)

FYI: When an issue is marked duplicate, issue queue etiquette states that you move the patch to the original issue. I have marked the other a dupe in this case.

Patch looks fine visually. However, when you remove a theme function, you will now also have to remove the function from the theme registry (hook_theme). System theme functions are included in common.inc. Setting to needs work.

I will test this later if need be.

Since you moved theme_pager_list into theme_pager and changed the parameters, your going to have to double check all the calls to theme('pager') and the theme registry for it to get the parameters lined up.

I'm not so sure about having the next/prev/first/last items showing when there's nothing to link to. I talked to EclipseGc about this and understand his points, just don't agree with it.

Everything else looks good. Keep it up. :)

Looks like this patch is going in the right direction, much more semantic!

One thing I can't tell from the patch is if it fixes this usability bug I opened almost 2 years ago... http://drupal.org/node/28138

Would love to see that go in now with this rewrite.

But EclipseGc, the visibility of pager list [1-9] varies which kills that argument. I don't see how the pager can be any bit more difficult if it changes.

Look at everything that's turned into a horizontal list. theme_links for example. It varies widely and I've never heard of any theming issues with that.

m3avrck: I generally like your idea to clean up any link clutter but it would mean hunting around with the mouse to target the relatively narrow links. Not sure which is more important.

I skimmed the code and the proposed changes look sane to me. Haven't tested the patch though. I'll test it when it is marked RTBC.

(If we can improve the PHPdoc of this function, or document the internals better, this seems like the right time to do it. Keep that in mind when massaging the code.)

• Not sure why $quantity is in the PHPDoc.
• There are a few indentation issues with array declarations.
• The whole idea of disabled links is regressive. It is IMO not usable either. -1. Also, by your reasoning, even if we have only 3 pages, we should display pages disabled links for pages 4 through 10 every time.

I think Maverick's request is sound. We are already more or less doing this anyways, except that we do this only on the first and last pages (and for both next and previous links).

Thanks :)
-K

I agree that this patch is a move in the right direction concerning semantic.

As for styling: I am not a fan of displaying disabled links in the pager.

It' true that some popular sites are doing this, but so far I've seen it in small pagers (2 to 7 links).
Drupal might show up to 14 items on the first page, with 4 of them disabled: first/prev/current page and ellipsis.
I am afraid this would be confusing.

Anyway, if displayed at all, the disabled links should be styled very clearly per default to make it apparent which is which (enabled/disabled/active).

BTW, the fact that all links are tagged "active" adds even more confusion.
I think that Zen's idea to style them separately from tabs was a good improvement.

Thanks
Riccardo

It currently displays empty list elements when first, prev, next and last are supposed to be hidden. The two array blocks for pager_next and pager_last also need to be brought into the if block above and additional checks added. Considering the extremely dodgy nature of $pager_last, you might need to use the $i counter. Please also check if the ellipses also need to be addresses for the same issue.

Thanks :)
-K

Re-rolled as per Zen's review: empty list elements for hidden controls are no longer generated.

Plus:
- slight optimization: no code is executed if pager is not needed.
- small PHPdoc corrections.

I did much testing and cannot find any differences from the original functionality (including ellipsis).

Thanks
Riccardo

The code looks good except for one thing: the use of $tags[2] to set the number of pages. This looks a bit hackish to me. Any particular reason for not making it a regular paramater that defaults to 9? If we don't need this, let's remove that hack. Why would modules want to communicate how many pages to list? Isn't that a theme thing? Weird.

@Dries: If we start looking at issues with pager.inc, we will be here for the rest of the year - it is something of a mess. Can we please fix the mark-up first and then look at cleaning up the rest of pager.inc in a separate issue?

Patch still applies, albeit with an offset; setting to RTBC.

Thanks,
-K

Eclipse: do you still intend to work on this?

Tested, reviewed and committed. Thanks folks!

Why the extra parameter for theme_pager? The $quantity value was originally derived from $tags:

// From theme_pager calling pager_list.
$output .= theme('pager_list', $limit, $element, ($tags[2] ? $tags[2] : 9 ), '', $parameters);


theme_pager_list($limit, $element = 0, $quantity = 5, $text = '', $parameters = array())

Now the $quantity is stuck as 9.

Whoops! excuse me. Noticed that all calls to theme_pager have a NULL set for $tags. Looks like it was there only for contrib modules.

But this should be documented form mod authors.

I actually WANT a version of pager that shows "Next/Previous" shaded out but still there when they are not "needed." Client just asked for it. It's confusing in some instances otherwise. So I'm bummed that the initial great idea here was totally snuffed out when that's what got me here in the first place. So I would like to have the option to leave in Next/Previous (even without a link) or take them out if desired. (Guess I will go back into the old patches here to find a version that takes it out for now.)

Figured it out - if you want to add ghost "previous/next" links on the first and last pages then you can just add some custom code as follows (and do the same for the function for the last page). This is not too hard to do, but it would be nice to have this as an option to change as desired depending on node:

function theme_pager_first($text, $limit, $element = 0, $parameters = array()) {
global $pager_page_array;
$output = '';

// If we are anywhere but the first page
if ($pager_page_array[$element] > 0) {
$output = theme('pager_link', $text, pager_load_array(0, $element, $pager_page_array), $element, $parameters, array('class' => 'pager-first'));
}

// ADDED
else {
$output = ' « first &nbsp ‹ previous';
}

return $output;
}

PS What I just added was based on the original pager.inc file.