Skip to content

Improve documentation pagination accessibility#26355

Merged
XhmikosR merged 2 commits intotwbs:v4-devfrom
ngyikp:improve-documentation-pagination-accessibility
Oct 23, 2018
Merged

Improve documentation pagination accessibility#26355
XhmikosR merged 2 commits intotwbs:v4-devfrom
ngyikp:improve-documentation-pagination-accessibility

Conversation

@ngyikp
Copy link
Copy Markdown
Contributor

@ngyikp ngyikp commented Apr 21, 2018

Fixes #22009:

  • Add rel="prev" and rel="next", also good for SEO!
  • Remove .sr-only span for previous/next page, there is already aria-label
  • Add aria-disabled="true" and aria-current="page"

Also fix the active state on the Sizing section, the current page number was using the disabled look, it should be the active look instead.

Comment thread docs/4.1/components/pagination.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

only information on these rel="..." attributes being of some use i came across in the past related to them being used on <link rel="..." ...> elements in the <head>. e.g. https://support.google.com/webmasters/answer/1663744?hl=en

if authors want to add this to their code, cool (assuming the attributes on actual links, rather than <link...> elements, does anything). but i don't think it's necessary for our example code

Comment thread docs/4.1/components/pagination.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see above

Comment thread docs/4.1/components/pagination.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see above

Comment thread docs/4.1/components/pagination.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see above

Comment thread docs/4.1/components/pagination.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would make more sense to add aria-disabled="true" to the actual link

Comment thread docs/4.1/components/pagination.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see above

Comment thread docs/4.1/components/pagination.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see above

Comment thread docs/4.1/components/pagination.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see above

Comment thread docs/4.1/components/pagination.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

as there's nothing focusable nor operable inside here, aria-disabled is mostly useless here

Comment thread docs/4.1/components/pagination.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would make more sense to add aria-disabled="true" to the actual link

@ngyikp ngyikp force-pushed the improve-documentation-pagination-accessibility branch from 5babc0e to 944a05c Compare April 23, 2018 03:14
@ngyikp
Copy link
Copy Markdown
Contributor Author

ngyikp commented Apr 23, 2018

Feedback addressed, please take another look

I added rel="prev" and rel="next" because another framework added them, I can't find much blogs/sites suggesting to put them on <a> tag instead of <link> either, so yeah, let's remove them.

@mdo
Copy link
Copy Markdown
Member

mdo commented Jul 20, 2018

Punting on this for v4.2 since I'm unsure of its status and we're hoping to launch v4.1.3 next week for some timely quality of life improvements and bug fixes.

@mdo
Copy link
Copy Markdown
Member

mdo commented Oct 21, 2018

@patrickhlauke Can you review and see if this is mergeable?

Copy link
Copy Markdown
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

LGTM

@patrickhlauke
Copy link
Copy Markdown
Member

(crikey, apologies for the multiple messages...was trying to combat github's hickup this morning, and thought they hadn't actually gone through)

- Remove `.sr-only` span for previous/next page, there is already`aria-label`
- Add `aria-disabled="true"` and `aria-current="page"`
@XhmikosR XhmikosR force-pushed the improve-documentation-pagination-accessibility branch from 944a05c to 29783e0 Compare October 22, 2018 12:20
Comment thread site/docs/4.1/components/pagination.md
@XhmikosR XhmikosR dismissed their stale review October 23, 2018 03:19

Should be like that

@mdo mdo mentioned this pull request Oct 23, 2018
@XhmikosR XhmikosR merged commit 47ef0a8 into twbs:v4-dev Oct 23, 2018
@pepelsbey
Copy link
Copy Markdown

@patrickhlauke, I know this thing is quite outdated, but still :) Wouldn’t it be easier to solve by removing a single href attribute instead of adding a bunch of ARIA with some extra CSS? I mean:

<li><a>1</a></li>
<li><a href>2</a></li>
<li><a href>3</a></li>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants