-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
tmplimpl: Add support for ellipsed paginator #3472
Conversation
See it in action here: http://bepsays.com/side/7/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a few small comments on accessibility, which I tend to take uber-seriously. That said, I think this is a smart/nice feature addition for the built-in template, so we don't need to let it hold up approval....
{{ end }} | ||
{{ if $showNumber }} | ||
<li | ||
{{ if eq . $pag }}class="active"{{ end }}><a href="{{ .URL }}">{{ .PageNumber }}</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{ 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">«</span></a> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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:
- AT screen readers and
aria-current
(Video) (Note this also shows tab indexing andaria-label
). - w3's
aria-current
, spec (Note section on pagination andpage
token) - 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?"
{{ end }} | ||
</ul> | ||
{{ end }}`) | ||
{{ if gt $pag.TotalPages 1 }} |
There was a problem hiding this comment.
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.
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fixes #3466