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.

Sign up for GitHub

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

Jump to bottom

tmplimpl: Add support for ellipsed paginator #3472

Merged
merged 1 commit into from
May 17, 2017
+41 −26
Merged

tmplimpl: Add support for ellipsed paginator #3472

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 41 additions & 26 deletions tpl/tplimpl/template_embedded.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,32 +123,47 @@ func (t *templateHandler) embedTemplates() {
`)

t.addInternalTemplate("", "pagination.html", `{{ $pag := $.Paginator }}
{{ if gt $pag.TotalPages 1 }}
<ul class="pagination">
{{ with $pag.First }}
<li>
<a href="{{ .URL }}" aria-label="First"><span aria-hidden="true">&laquo;&laquo;</span></a>
</li>
{{ end }}
<li
{{ if not $pag.HasPrev }}class="disabled"{{ end }}>
<a href="{{ if $pag.HasPrev }}{{ $pag.Prev.URL }}{{ end }}" aria-label="Previous"><span aria-hidden="true">&laquo;</span></a>
</li>
{{ range $pag.Pagers }}
<li
{{ if eq . $pag }}class="active"{{ end }}><a href="{{ .URL }}">{{ .PageNumber }}</a></li>
{{ end }}
<li
{{ if not $pag.HasNext }}class="disabled"{{ end }}>
<a href="{{ if $pag.HasNext }}{{ $pag.Next.URL }}{{ end }}" aria-label="Next"><span aria-hidden="true">&raquo;</span></a>
</li>
{{ with $pag.Last }}
<li>
<a href="{{ .URL }}" aria-label="Last"><span aria-hidden="true">&raquo;&raquo;</span></a>
</li>
{{ end }}
</ul>
{{ end }}`)
{{ if gt $pag.TotalPages 1 }}
Copy link
Contributor

@rdwatters rdwatters May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption is that this would be a breaking change, but really this needs to be wrapped in <nav role="navigation" aria-label="Pagination Navigation">. Only mentioning this here as a note that I should add this to the new docs.

<ul class="pagination">
{{ with $pag.First }}
<li>
<a href="{{ .URL }}" aria-label="First"><span aria-hidden="true">&laquo;&laquo;</span></a>
</li>
{{ end }}
<li
{{ if not $pag.HasPrev }}class="disabled"{{ end }}>
<a href="{{ if $pag.HasPrev }}{{ $pag.Prev.URL }}{{ end }}" aria-label="Previous"><span aria-hidden="true">&laquo;</span></a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tricky accessibility issue: do we keep "Previous" if it means including English assistive text in multilingual sites?

If yes, we should likely add aria-label's to each of the anchors; e.g. aria-label="Go to Page {{.PageNumber}}".

If this is in any way a breaking change, however, I'm not too adamant about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hearing from the blind person in the forum, I think we overvalue the labels. I suspect that it gets noisy when doing too much. If a blind person "sees" a sequence of integer links, he/she understands that it is a paginator.

And in general, your remarks are about the existing code. This PR should focus on the changes made.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we overvalue the labels. I suspect that it gets noisy when doing too much. If a blind person "sees" a sequence of integer links, he/she understands that it is a paginator.

I wouldn't presume to know; hence why I defer to standards built for AT.

And...

...I'm not too adamant about it

LGTM 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can look at this later ... would nice if you could post a link to those standards? (but I suspect those standards are written by people who see)

Copy link
Contributor

@rdwatters rdwatters May 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...would nice if you could post a link to those standards?

Sure:

  1. AT screen readers and aria-current (Video) (Note this also shows tab indexing and aria-label).
  2. w3's aria-current, spec (Note section on pagination and page token)
  3. Accessible pagination (A11Y)

but I suspect those standards are written by people who see

I agree we can table these as maybe "future iterative improvements?"

</li>
{{ $.Scratch.Set "__paginator.ellipsed" false }}
{{ range $pag.Pagers }}
{{ $right := sub .TotalPages .PageNumber }}
{{ $showNumber := or (le .PageNumber 3) (eq $right 0) }}
{{ $showNumber := or $showNumber (and (gt .PageNumber (sub $pag.PageNumber 2)) (lt .PageNumber (add $pag.PageNumber 2))) }}
{{ if $showNumber }}
{{ $.Scratch.Set "__paginator.ellipsed" false }}
{{ $.Scratch.Set "__paginator.shouldEllipse" false }}
{{ else }}
{{ $.Scratch.Set "__paginator.shouldEllipse" (not ($.Scratch.Get "__paginator.ellipsed") ) }}
{{ $.Scratch.Set "__paginator.ellipsed" true }}
{{ end }}
{{ if $showNumber }}
<li
{{ if eq . $pag }}class="active"{{ end }}><a href="{{ .URL }}">{{ .PageNumber }}</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should have the following, @bep, for accessibility and a11:

{{ if eq . $pag }}class="active" aria-current="true"{{ end }}><a href="{{ .URL }}">{{ .PageNumber }}</a></li>

(Reference)

{{ else if ($.Scratch.Get "__paginator.shouldEllipse") }}
<li class="disabled"><span aria-hidden="true">&hellip;</span></li>
{{ end }}
{{ end }}
<li
{{ if not $pag.HasNext }}class="disabled"{{ end }}>
<a href="{{ if $pag.HasNext }}{{ $pag.Next.URL }}{{ end }}" aria-label="Next"><span aria-hidden="true">&raquo;</span></a>
</li>
{{ with $pag.Last }}
<li>
<a href="{{ .URL }}" aria-label="Last"><span aria-hidden="true">&raquo;&raquo;</span></a>
</li>
{{ end }}
</ul>
{{ end }}`)

t.addInternalTemplate("", "disqus.html", `{{ if .Site.DisqusShortname }}<div id="disqus_thread"></div>
<script type="text/javascript">
Expand Down