Full Pager: The ellipsis ("...") button does nothing

That button is controlled and generated by theme('pager') which is Drupal core. Views has no control or influence over it.

The patch attached will add theme_pager_ellipsis which returns a themed pager_link.
When displaying pages 3 -11 the first ellipsis (after

This should go into the current development branch of Drupal (8.x-dev) and then backported if needed. Also settting to needs review for patchbot to find it.

Looks like a good addition though!

This is a patch for D8 based on teh patch in #3

are these patches the same?

I'm not sure why jasonsavino posted #7 ... it looks like the only difference between the two is the position within the files that the new functions were added

Code works as advertised (either patch). Can we add a tooltip on the ellipses like "Jump to page x" like we have on the other pager links?

I added an attributes array to the theme function and passed "t('Jump to previous group')" and "t('Jump to next group')" to the ellipsis links.

Thanks jasonsavino, works for me.

+++ b/core/includes/pager.incundefined
@@ -466,6 +466,31 @@ function theme_pager_first($variables) {
+  $text = $variables['text'];
+  $element = $variables['element'];
+  $interval = $variables['interval'];
+  $parameters = $variables['parameters'];
+  $attributes = $variables['attributes'];

Why bother assigning all these variables? Why not just use the long form in the theme function?

An excellent point tim

This cleans up that theme function a bit

Thanks Alex. Works for me.

woot

Hmm, I'm not sure why we can't prepare the variables in theme_pager() and use theme_pager_link() here too, is there a reason to add the new theme function otherwise?

I think the extra theme function is more about making it easier to override by themers ... With the pager in particular there is a history of generalizing the different pieces (like class names) and we end up coming back to it later anyway.

Is there a compelling argument not to have the new theme function?

I'm totally with bleen18 on this.

I have tested and everything works like it supposed to.

For specific overrides, we ought to be able to use theme hook suggestions no? So theme('pager_link__ellipsis')?

Catch's suggestion in #20 seems to make sense.... so I took it one step further. I replaced all the theme_pager_* functions with theme suggestions based off of theme_pager_link. With this change a user can add any of the following functions:

• mytheme_pager_link($variables){ ... }
• mytheme_pager_link__first($variables){ ... }
• mytheme_pager_link__last($variables){ ... }
• mytheme_pager_link__previous($variables){ ... }
• mytheme_pager_link__next($variables){ ... }
• mytheme_pager_link__ellipsis($variables){ ... }

.. or the corresponding tpl instead.

I was a bit concerned about scope creep with this patch, but I spoke with catch in IRC and he agreed that as long as we are cleaning things up then he wouldn't object to it.

... now, let's see what testbot has to say

+++ b/core/includes/pager.inc
@@ -583,6 +458,7 @@ function theme_pager_last($variables) {
+ *   - suggestion: The theme suggestion that should be attempted.

Hmm..., I don't see that being used in the patch. Or is that some hidden feature by our theme system, that I didn't know of :)

Otherwise looks pretty cool. Nice diffstat!!!
Could use someone trying this out, though.

+++ b/core/includes/pager.inc
@@ -354,10 +354,28 @@ function theme_pager($variables) {
+    $li_first = theme('pager_link__first', array('text' => (isset($tags[0]) ? $tags[0] : t('« first')), 'page_new' => $page_new, 'element' => $element, 'parameters' => $parameters));

And below: Since we're already modifying this in the patch, can we change this to use the verbose array declaration split over multiple lines. The patch doesn't make things worse than they were, but it's still very hard to read.
(That will hurt the diffstat, but still... :) )

re #22: That was some cruft left over from a previously failed attempt. I've killed it with fire.

re #23: I spose...

That looks so much better. Code-wise this is RTBC. I don't know in how good our test coverage is with regards to pagers, but I guess someone trying this out locally can't hurt.

Ahem. The massive code clean-up here looks really nice and I totally like that.

But the actual suggestion of turning the "..." into something clickable sounds very odd to me. I don't think I've ever seen such a pager feature/behavior anywhere else on the net.

In fact, I totally would not expect that clicking on "..." will effectively load a new page which is the same as the "next" link, but just moves/shifts the pager items/numbers differently.

I think what I'd rather expect would be some fancy simple JS that only moves/shifts the pager items/numbers, but without reloading the entire page, so I'm able to get to page 23 of 42 more quickly. (Overall that's an edge-case of a use-case on its own.)

I think this proposal needs more design/usability discussion. Therefore, I'd almost suggest to move that nice code clean-up work into a separate issue.

Since this removes not one, not two, not three, but FOUR theme functions I'm tentatively tagging it with the theme system cleanup tag.

@sun - instead of jumping to the same page as "next", how about jumping further ahead? Then it has a more specific function that we currently don't offer - a way to jump far ahead without just editing the pagenum in the url.

....

Not sure, what I should review.

I believe we have added to much to the initial request. The request was to add clickability to the ellipsis'. That was done and should stop there. If there is an issue with the way pager theme functions are done, shouldn't that be it's own issue?

@sun - to your comments in #26, to be honest I find it odd that we would have a the ellipsis at all. If there are next/previous links then logic dictates that there must be more pages. When the next or previous links disappear then there are no pages ... could it get any more simple?
But since they are there, they should be useful. To that end the patch in #11 allows the user to jump to the next set of links. If you had, for instance, 30 pages worth of content, the page would show 9 pages and the ellipsis link would jump you so you would see 10 - 18 (roughly). This is not the same functionality as "next" which only moves you one page.

Totally agreed. Moved the clean-up into #1598886: Clean up pager theme functions

> If you had, for instance, 30 pages worth of content, the page would show 9 pages and the ellipsis link would jump you so you would see 10 - 18 (roughly).

'Roughly'? Could someone explain exactly what clicking the ellipsis would do? Perhaps use the drupal core issue queue as an example: what happens when I click either of the two ellipses on this page: http://drupal.org/project/issues/drupal?page=8

http://www.webdistortion.com/2008/10/27/26-pager-styles-from-around-the-...

I tend to agree with @sun on this one, it seems like we are trying to be smart creating a interaction that would be relatively unique to Drupal. I don't even get why we have this in core, the use of ellipsis tends to be between the last highest, and the last page - not before next/last.

Hi all, just fyi, I recently contributed the Pagerer module http://drupal.org/project/pagerer , that provides some pager themes that help addressing pages far away from current. take a look if you wish.

LOL. Awesome module name.

#24: 1190436.patch queued for re-testing.

Most of the patch in #24 was moved (and committed) to #1598886: Clean up pager theme functions

I'm not surprised it fails now

updating tags.
needs reroll. http://drupal.org/patch/reroll (or just remaking a patch manually making the changes that are still needed.)

add back the needs screenshot tag when a new patch is posted.

Attempted reroll from #24 patch (date: May 18, 2012) - but unable to continue with rebase ... (bullet point 11.) "... you might want to skip this patch."?

drupal8sb> git status
# Not currently on any branch.
# You are currently rebasing.
#   (fix conflicts and then run "git rebase --continue")
#   (use "git rebase --skip" to skip this patch)
#   (use "git rebase --abort" to check out the original branch)
#
# Unmerged paths:
#   (use "git reset HEAD <file>..." to unstage)
#   (use "git add <file>..." to mark resolution)
#
#       both modified:      core/includes/common.inc
#       both modified:      core/includes/pager.inc
#
no changes added to commit (use "git add" and/or "git commit -a")
drupal8sb> git add core/includes/common.inc
drupal8sb> git add core/includes/pager.inc
drupal8sb> git status
# Not currently on any branch.
# You are currently rebasing.
#   (all conflicts fixed: run "git rebase --continue")
#
nothing to commit, working directory clean
drupal8sb> git rebase --continue
Applying: Applying patch from http://drupal.org/node/1190436#comment-6012018
No changes - did you forget to use 'git add'?
If there is nothing left to stage, chances are that something else
already introduced the same changes; you might want to skip this patch.

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
drupal8sb> 

Rerolled with same result after fresh install ...

Attached are the two "conflict" patches for core/includes/common.inc and core/includes/pager.inc the only two conflict files identified during the rebase.

Hope this helps? ;)

Made changes manually as mentioned by YesCT in comment #40.

It shouldn't work.

@manu4543 - thanks for working on this :) I think the rerolls are missing changes from earlier patches. The patch in #24 has 2 files changed, 64 insertions, 167 deletions.

The patch in #43 has 1 files changed, 26 insertions, 13 deletions.

So I would recommend a straight reroll of #24 and resolving conflicts during the rebase, instead of trying to recreate the patch manually.

I dont know much about theming, but...

code style looks ok, on a quick look.

At first it looked like two identical blocks of code were added in different places, but they are different: one for next group, one for previous group.

I think the character change from ... to uh, something else might have been unintentional.

(http://drupal.org/project/dreditor helped me do this review)

Also, interdiffs are great.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff Or, http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...

since this applies now, removing the needs reroll tag.

@Cottser good points!

I'm going to try and remember to compare the number of
files changed, insertions, deletions
when I look at future rerolls.

@Cottser, @YesCT - Most of the patch from #24 has been committed see 39

Ah I totally missed #39, sorry about that.

Nothing fancy, just removed the uh (?) symbol.

+++ b/core/includes/pager.incundefined
@@ -253,7 +253,13 @@ function theme_pager($variables) {
+            'text' => '…',

I still contend that we should be using … here

@bleen18 - Wait, I'm confused. We should or shouldn't be using an ellipsis character? Both before and after the patch in #53 it's an ellipsis character, not three dots.

Blarg ... I meant … but it got rendered in my comment

Thisis RTBC?

Manually tested by applying the patch in #53 and everything looks working fine. Attached screenshot of default and click of ellipsis.

Setting it RTBC...

No one has answered my question ... Why aren't we using … ...

Because we already use ... in several places, and we don't use hellip anywhere yet in Drupal code.
But a follow-up issue to replace it everywhere would be great.

...here you go: #2018591: Replace all instances of ... with …

We should have a test that clicks on the elipsis and confirms it works.

contributor task document on how to add tests: https://drupal.org/contributor-tasks/write-tests

updated issue summary with follow-up and remaining tasks

Adding test case...

lets get a tests only patch without the fix, and make sure the testbot shows it fails.

Here is the same patch in #64 with test_only

thanks. that looks good.

Just a small clean up to
make function use third person verb in the comment description.

Very nice work people :) a couple more minor docs touchups to be done:

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerTest.phpundefined
@@ -69,6 +67,24 @@ function testActiveClass() {
+    // check for Ellipsis.

"Check for ellipsis." would be better here - capital C and ellipsis is not a proper noun.

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerTest.phpundefined
@@ -69,6 +67,24 @@ function testActiveClass() {
+    // Make sure it is click-able.

clickable instead of click-able.

Thanks for your review @Cottser. Here is the fix.

#69: 1190436-ellipsis-does-nothing-69.patch queued for re-testing.

#69: 1190436-ellipsis-does-nothing-69.patch queued for re-testing.

Wow, that was quick @vijaycs85 :)

+++ b/core/includes/bootstrap.incundefined
@@ -2469,7 +2469,7 @@ function language_list($flags = Language::STATE_CONFIGURABLE) {
-    $default = language_default();
+    $default = languagePagerTest.php_default();

This change seems unintentional.

+++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerTest.phpundefined
@@ -76,10 +76,10 @@ function testEllipsisLink() {
-    // check for Ellipsis.
+    // Check for Ellipsis.

Lower 'e' for ellipsis…

We also lost @YesCT's changes from #67 in the latest patch.

added follow up and remaining tasks

Rerolled #67 to test prior to full review (#68 to #70.0) and then update

Patch does not appear to display ellipsis (hence test failure?) using replicated settings from a Drupal 7 site (similar 300 Devel'd pages) that correctly displays (inactive ;) ellipsis.

No longer an issue in either Drupal 7 or 8

I got the same issue in drupal 9.4.8.

Ellipsis appear before Last button in pager. I want to remove ellipsis since it does nothing.

Any solution to this?

I tested this on Drupal 10.1.x, standard install using the steps in the issue summary. i was able to reproduce the problem.

Yeah this is still valid. Can we just drop the ellipsis entirely?

I am not really sold on this change, I think that the point of it is a visual indication that there are more pages. I don't think it needs to do anything beyond that, and I certainly don't think it's safe to assume that whatever we have it do is what a user would expect.

Given that the current simple visual indicator is not at all accessible, I would also support just removing it, although it is possible some sites expect it to be there right?