Patch #3 in my series of Drupal-markup-spaghetti-core fixes, addresses the pager, found on the bottom of every page that has multiple pages.

This patch fixes the following things:

- markup is reduced *significantly*, no more extraneous

or other meanlingless markup
- improves themability through semantic markup and more accessible CSS classes
- adds a title link to each pager link, accessibility improvement
- adds visual aids to first, previous, next, and last links ... this dramatically increases usability, and cleans up the clutter with pager links (this falls in line with the most common trends with pagination, see Google, Digg, etc..)
- renamed attributes to parameters so that HTML attributes could be added to each themable function
- cleans up pager code a bit

Tested and looking great! I must say, I don't *dread* looking at pager links at the bottom anymore, they look visually more appealing (no more weird spacing breaks either) and I know underneath the code isn't a mess!

Stay tuned for patch #4...

CommentFileSizeAuthor
#10 pager_0.diff3 KBSteven
#8 pager.png2.62 KBm3avrck
#5 drupal_34.patch12 KBm3avrck
drupal_33.patch12.05 KBm3avrck

Comments

m3avrck’s picture

m3avrck commented

Oh forgot to add, by using this improved markup structure, themers should have a much *easier* time, as they won't have to override all of these Drupal theme functions just to clean them up. They'll be looking pretty "sexy" by default now ;-)

m3avrck’s picture

m3avrck commented
Status: Reviewed & tested by the community » Needs review

Oops, let's have this set to review just in case.

drumm’s picture

drumm
he/him
Location NY, US
commented 
-        $output .= '<strong>'. $i .'</strong> ';
+        $output .= '<span class="pager-current">'. $i .'</span>';

I think we should keep <strong>, but add that class.

m3avrck’s picture

m3avrck commented

I added the font-weight: bold for that class to the CSS. However, that is a good point, instead of just a meaningless <span> a <strong> would be better. I'll reroll the patch in a bit.

m3avrck’s picture

m3avrck commented
StatusFileSize
new drupal_34.patch12 KB

updated patch keeping <strong> in there now but with pager-current class, more semantic than <span>.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community
dries’s picture

dries commented

- renamed attributes to parameters so that HTML attributes could be added to each themable function

I don't understand this change.

Is the HTML that much better/smaller? Is it possible to post before and after HTML code?

m3avrck’s picture

m3avrck commented
StatusFileSize
new pager.png2.62 KB

Dries, the renaming was to be consistent with core. To add the appropriate titles to each pager link, I needed to pass in an $attributes('title' => 'title goes here'), however because of the way the functions were setup, they were already using the name $attibutes, which I thought was odd. Generally in core it seems anytime you encounter $attributes it is for $attributes on some sort of HTML markup, whether it be a link, table, form, etc... I can revert this change if you want, but do you have a name I can use that *isn't* $attributes then so I can pass in the HTML attributes?

Attached is a screen shot, before and after, note how much cleaner it looks and how much easier it is, usability wise.

As for the code, here is before:

<div id="pager" class="container-inline">
  <div class="pager-first"><a href="?q=node" class="active">first page</a></div>
  <div class="pager-previous"><a href="?q=node&amp;page=3" class="active">previous page</a></div>
  <div class="pager-list">
    <div class="pager-previous"><div class="pager-first"><a href="?q=node" class="active">1</a></div></div> 
    <div class="pager-previous"><a href="?q=node&amp;page=1" class="active">2</a></div> 
    <div class="pager-previous"><a href="?q=node&amp;page=2" class="active">3</a></div> 
    <div class="pager-previous"><a href="?q=node&amp;page=3" class="active">4</a></div>
    <strong>5</strong> 
    <div class="pager-next"><a href="?q=node&amp;page=5" class="active">6</a></div>
    <div class="pager-next"><a href="?q=node&amp;page=6" class="active">7</a></div> 
    <div class="pager-next"><a href="?q=node&amp;page=7" class="active">8</a></div>
    <div class="pager-next"><a href="?q=node&amp;page=8" class="active">9</a></div> 
    <div class="pager-list-dots-right">...</div>
  </div>
  <div class="pager-next"><a href="?q=node&amp;page=5" class="active">next page</a></div>
  <div class="pager-last"><a href="?q=node&amp;page=28" class="active">last page</a></div>
</div>

And after:

<div id="pager">
  <a href="?q=node" class="pager-first active" title="goto first page">&#171; first</a>
  <a href="?q=node&amp;page=3" class="pager-previous active" title="goto previous page">&#8249; previous</a>
  <span class="pager-list">
    <a href="?q=node" class="pager-first active" title="goto page 1">1</a>
    <a href="?q=node&amp;page=1" class="pager-previous active" title="goto page 2">2</a>
    <a href="?q=node&amp;page=2" class="pager-previous active" title="goto page 3">3</a>
    <a href="?q=node&amp;page=3" class="pager-previous active" title="goto page 4">4</a>
    <strong class="pager-current">5</strong>
    <a href="?q=node&amp;page=5" class="pager-next active" title="goto page 6">6</a>
    <a href="?q=node&amp;page=6" class="pager-next active" title="goto page 7">7</a>
    <a href="?q=node&amp;page=7" class="pager-next active" title="goto page 8">8</a>
    <a href="?q=node&amp;page=8" class="pager-next active" title="goto page 9">9</a>
    <span class="pager-ellipses">&#8230;</span>
  </span>
  <a href="?q=node&amp;page=5" class="pager-next active" title="goto next page">next &#8250;</a>
  <a href="?q=node&amp;page=28" class="pager-last active" title="goto last page">last &#187;</a>
</div>

Note how much cleaner the code is, not extraneous

in there.

If you are wondering about all of the class="active", look at this bug: http://drupal.org/node/44502

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Looks good indeed. Committed to HEAD. Thanks.

Steven’s picture

Steven commented
Status: Fixed » Needs review
StatusFileSize
new pager_0.diff3 KB 
-  return l($text, $_GET['q'], array(), count($query) ? implode('&', $query) : NULL);
+  return l($text, $_GET['q'], $attributes, count($query) ? implode('&', $query) : NULL, NULL, FALSE, TRUE);

This code introduces possible HTML issues because the pager title is no longer escaped. If someone changes the text to ">>" or "<<" (which is not inconceivable) there will be broken HTML.

The reason seems to be the unnecessary and ugly use of HTML entities. We should use literal characters instead. It makes the code more readable, and avoids hacks like:
$attributes['title'] = is_numeric($text) ? 'goto page ' . $text : 'goto ' . str_replace(array('&#171; ', '&#8249; ', ' &#8250;', ' &#187;'), '', $text) . ' page';

Finally, the text 'goto page' is not translatable. Shouldn't it be "go to" anyhow?

m3avrck’s picture

m3avrck commented
Status: Needs review » Reviewed & tested by the community

Steven, great catch! Yes, I always seem to forget that Drupal is UTF-8 so we can use literals, I do like that change very much! And the catch on the titles was very good too, completely slipped my mind.

As for the 'goto' verse 'go to', I actually debated this myself. Here in America I see it used both ways all the time, not sure if there is the *real* proper way or not, so let's go with what you have and keep it seperate.

RTC!

dries’s picture

dries commented

Feel free to commit.

Steven’s picture

Steven commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)