Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should have real first/last page links, and these should be used by default instead of outer window #30

Closed
GarthSnyder opened this issue Feb 23, 2011 · 8 comments

Comments

@GarthSnyder
Copy link

I was looking into some more details of will_paginate's behavior related to :outer_window, and it occurs to me that there's a more basic problem with the current versions of both will_paginate and kaminari: neither seems to properly support "first page" and "last page" links in the default configuration.

Will_paginate-style outer windows are in fact uncommon in the wild, and they only really exist to cover the lack of a proper "first" and "last". In this light, will_paginate's definition of the outer windows as "extra pages around the first and last pages" is more understandable. But, it's also an opportunity for kaminari to do things right by reflecting the predominant styles used on the web in its defaults.

Take a look at this nice set of example paginators from Smashing Magazine (via the will_paginate docs). Although the specific elements that are present varies, the order of presentation is invariant:

[first] [previous] 1 2 ... 5 6 [7] 8 9 ... 12 13 [next] [last]

Of particular note, a "first page" link, if present, should always precede the "previous page" link, and vice-versa for last/next. The outer windows cannot properly substitute for these links because by definition they appear interior to "previous" and "next". Furthermore, "first" and "last" links should appear in addition to the page numbers of the first and last pages if you are using an outer window, as shown above. (If you were on page 12, "13", "next", and "last" might all point to the same page; that's perfectly fine.)

A complete paginator specification requires answers to all of the following questions:

  1. Should a "first page" link be displayed?
  2. If so, should it be omitted when you are on the first page (vs. displayed but disabled)?
  3. Should a "previous page" link be displayed?
  4. If so, should it be omitted when you are on the first page (vs. displayed but disabled)?
  5. Should the first N pages always be displayed? How many?
  6. How many pages surrounding the current page should be displayed?
  7. Should the last N pages always be displayed? How many?
  8. Should a "next page" link be displayed?
  9. If so, should it be omitted when you are on the last page (vs. displayed but disabled)?
  10. Should a "last page" link be displayed?
  11. If so, should it be omitted when you are on the last page (vs. displayed but disabled)?

To cover the full range of options, I propose the following modifications to paginate():

  • :window, :outer_window, :left, :right would be interpreted similarly to how they are now, except the supplied N would be the number of pages you actually get. 0 means disable for the outer windows.
  • Outer windows would be disabled by default in favor of first/last links.
  • Currently, "next" and "previous" links are always generated and their labels come from the translation database. I suggest making :next and :previous params to paginate(), with the same default values; if either is nil, don't generate the corresponding span.
  • Add :first and :last params to paginate() with suitable (present) defaults. If set to nil, these suppress the first and last links.
  • Currently, "next" and "previous" are rendered as empty spans (no link, no text) when they point to the current page. This is hard to style and inconsistent. Better: include a "disabled" or "inactive" class when appropriate, and always include the label text (but no link tag when current page). Users that want these tags to disappear completely when inapplicable can style them to display: none. (Similarly for "first" and "last".)
  • The double angle quotes currently used in the default labels for previous and next are potentially confusing because they bind more naturally to "first" and "last" than to "previous" and "next". (Consider a standard set of video or audio transport controls: << < stop > >>) Better: "Prev" and "Next" or "< Prev" and "Next >"

Under this regime, the defaults would be:

:window       => 3 or 4 (currently 4)
:outer_window => 0 unless :first == nil or :last == nil; in that case, 2
:next         => raw(t 'views.pagination.next'), with default translation "Next >"
:previous     => raw(t 'views.pagination.previous'), with default translation "< Prev"
:first        => raw(t 'views.pagination.first'), with default translation "<< First"
:last         => raw(t 'views.pagination.last'), with default translation "Last >>"

Some examples:

= paginate @things
[<< First] [< Prev] ... 6 7 8 [9] 10 11 12 ... [Next >] [Last >>]

= paginate @things :first => nil, :last => nil
[< Prev] 1 2 ... 6 7 8 [9] 10 11 12 ... 16 17 [Next >]

= paginate @things, :previous => nil, :next => nil
[<< First] ... 6 7 8 [9] 10 11 12 ... [Last >>]

= paginate @things, :first = nil, :left => 2, :window => 2
[< Prev] 1 2 ... 7 8 [9] 10 11 ... [Next >] [Last >>]

= paginate @things, :first => "First Page", :previous => "Previous", :next = "Next", :last => "Last Page"
 [First Page] [Prev] ... 6 7 8 [9] 10 11 12 ... [Next] [Last Page]
@amatsuda
Copy link
Member

Thank you for detailed research and suggestion. This is really helpful!

I've briefly finished the implementation except the helper options such as :first => nil. Can you please check the issue_30 branch? https://github.com/amatsuda/kaminari/tree/issue_30

@GarthSnyder
Copy link
Author

Thanks for working on this! I think it's looking very nice.

I see no problems with the mechanics so far. I tried a variety of combinations of inner and outer windows, and all the regions and links were all perfect.

Now that I've had the chance to see the generated HTML for the entire nav section under this modified system, I do have a few minor comments about classes and styling hooks and such.

First, I'm coming to think that my suggestion in Issue 28 ("page can be both current and first"), which you so rapidly and graciously integrated, may have been misguided. Or more specifically, I think it was a good idea in the original context where "page 1" was trying to serve as a First link, but now that a real First link is available, I'm starting to think that individual page numbers should not be tagged with the "first" or "last" classes.

The point of those classes was to provide hooks for special styling on the first and last pages, presumably so they could be treated more like First and Last buttons. But now that there are real First and Last links, I'm having a hard time thinking of a scenario in which you'd want to have the corresponding numbers specially styled.

The drawback to "first" and "last" classes on page numbers is that it complicates styling since the CSS selectors "span.first" and "span.last" then correspond to more than one kind of entity.

Along the same lines, <a> tags don't need to include classes since their enclosing span elements already bear all the relevant information. The selector "span.last a" lets you target the Last link, for example.

I noticed that some of the links emitted by the paginator include a rel attribute, but this convention isn't carried through to the individual pages. According to W3C, there is also a "start" rel type in addition to "next" and "prev" types (but curiously, no "last" or "end"). If you're going to provide rels, you might as well do it everywhere. Pages can have multiple copies of the paginator, so browsers already have to deal with multiple rels of the same type.

Finally, didn't there used to be a "truncate" or "gap" class on the "..." elements? This seems useful for cases like this where everything has a certain styling except those gaps:

Example paginator with gap

I pushed a clone that addresses the issues outlined above; I'll send a pull request. Here's a complete example of the generated HTML (from page 2 of 28):

<nav class='pagination'>

  <span class='first'>
    <a href="/books" rel="start prev">&laquo; First</a>
  </span>
  <span class='prev'>
    <a href="/books" rel="start prev">&lsaquo; Prev</a>
  </span>

  <span class='page'>
    <a href="/books" rel="start prev">1</a>
  </span>
  <span class='page current'>2</span>
  <span class='page'>
    <a href="/books&page=3" rel="next">3</a>
  </span>
  <span class='page'>
    <a href="/books&page=4">4</a>
  </span>

  <span class='page gap'>
    ...
  </span>

  <span class='page'>
    <a href="/books&page=27">27</a>
  </span>
  <span class='page'>
    <a href="/books&page=28">28</a>
  </span>

  <span class='next'>
    <a href="/books&page=3" rel="next">Next &rsaquo;</a>
  </span>
  <span class='last'>
    <a href="/books&page=28">Last &raquo;</a>
  </span>

</nav>

@amatsuda
Copy link
Member

Thank you very much for your work! Just merged your pull request.

First, (snip) individual page numbers should not be tagged with the "first" or "last" classes.

Agree. Took your commit.

Along the same lines, tags don't need to include classes

You're right. This is not needed.

If you're going to provide rels, you might as well do it everywhere.

I'm not sure, but who needs rel="start"? I think rel="next" is very useful because there are many browsers or browser plugins using this attribute, but I can't see rel="start"'s use case. I prefer simpler code to that minor attribute.

Finally, didn't there used to be a "truncate" or "gap" class on the "..." elements?

Thank you for adding this :)

@GarthSnyder
Copy link
Author

I have no evidence that anyone in the world would actually use rel="start", so that sounds perfectly reasonable.

I looked around for a bit, but rel= doesn't seem to be covered in any of the usual best practices guidelines. The original intent may have been to support accessibility, but these days most concern with rel= seems to relate to SEO and robot control. So it sounds like you're making the right tradeoff.

Thanks for looking this over!

@amatsuda
Copy link
Member

Garth,

Sorry for leaving this issue for a whole week.

I have no evidence that anyone in the world would actually use rel="start", so that sounds perfectly reasonable.

OK, then let's not use rel="start" by default.

One another thing I'm worrying about is this one.

Currently, "next" and "previous" are rendered as empty spans (no link, no text) when they point to the current page. This is hard to style and inconsistent. Better: include a "disabled" or "inactive" class when appropriate, and always include the label text (but no link tag when current page). Users that want these tags to disappear completely when inapplicable can style them to display: none. (Similarly for "first" and "last".)

Yes, this works beautifully as you shown above, but this means, "first", "prev", "next" and "last" are always shown unless you style them properly.
Doesn't this look too messy as the default view?

« First ‹ Prev 1 2 3 4 5 ... Next › Last »

So, i'm thinking of defaulting "empty spans" and push the "always showing the labels" version to an example theme. What do you think?

@GarthSnyder
Copy link
Author

I'm finding these issues very interesting to think about. Thanks again for engaging with me on this - I hope you find the comments useful!

I am 100% with you on the principle that the default behavior should be as close to the average person's requirements as possible. It does seem like a bad idea to require styling to get proper default behavior. I also agree that displaying multiple disabled/nonfunctional elements seems like a sub-optimal default. I surveyed some random web sites, and although some sites do show disabled first/prev/next/last elements, the majority do not.

My objection to empty spans for inapplicable first/prev/next/last links is that there's not much you can do with them even if you do decide you want to style them. They don't include the translated text for the link, and there's not even a clear way to target them with a CSS selector. (You can target "the 'first' link", but not "the 'first' link in its disabled/inapplicable/empty state".) Can you think of a scenario in which an empty span might be useful?

Taking a step back, it seems pretty clear that the theming system needs to include support for more than just partials. We talked about theme-specific images a while ago, but theme-specific CSS and JavaScript files would also be needed to enable drop-in themes that mimic the predominant paginator styles. Perhaps the best long-term solution would be to enrich themes to incorporate these elements and then include a CSS rule in the default theme that hides disabled elements. (I haven't worked on a Rails engine myself, but if I understand the concept correctly, these extra components should be pretty well supported.)

In the interim, perhaps the disabled items should simply not be emitted, even as an empty span. If you want to force those items to display, you can edit the partial.

amatsuda added a commit that referenced this issue Apr 13, 2011
… empty span"

"If you want to force those items to display, you can edit the partial."
amatsuda added a commit that referenced this issue Apr 19, 2011
…ead of outer window

* :window, :outer_window, :left, :right will be interpreted similarly to how they were, except the supplied N would be the number of pages you actually get. 0 means disable for the outer windows
* outer windows will be disabled by default in favor of first/last links
* include a "disabled" class when appropriate, and always include the label text (but no link tag when current page). Users that want these tags to disappear completely when inapplicable can style them to display: none. (Similarly for "first" and "last".)
* changed the default labels - double angle quotes for "first" and "last", single angle quotes for "previous" and "next". (Consider a standard set of video or audio transport controls: << < stop > >>)
amatsuda added a commit that referenced this issue Apr 19, 2011
* do not output rel="start" by default
* do not output rel on first_page and last_page
amatsuda added a commit that referenced this issue Apr 19, 2011
amatsuda added a commit that referenced this issue Apr 19, 2011
amatsuda added a commit that referenced this issue Apr 19, 2011
@amatsuda
Copy link
Member

Finally pushed 0.11.0 gem containing all these changes. Thanks!

@thiyagarajan
Copy link

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants