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

Add accessibility roles to pagination nav #848

Merged
merged 2 commits into from Jan 21, 2017

Conversation

frrrances
Copy link
Contributor

This PR adds an aria navigation role and a pager aria label for the nav element to improve accessibility.

@yuki24
Copy link
Member

yuki24 commented Jan 12, 2017

Thank you for your pull request. None of the maintainers including myself are familiar with accessibility. Could you elaborate on that? Also, we no longer accept feature requests for slim or haml templates. Could you remove the changes in the slim/haml templates from the commit?

@frrrances
Copy link
Contributor Author

Sure thing - adding an aria role attribute to the nav element makes is much easier for screenreaders to find and use the navigation and it's what the W3C suggests:

https://www.w3.org/WAI/GL/wiki/Using_HTML5_nav_element

Adding a label let's the user differentiate between the different navs on the page. Here's a little more about marking up navigation for good accessibility:

http://www.a11ymatters.com/pattern/pagination/

And I've removed the changes to the slim and haml templates.

@yuki24
Copy link
Member

yuki24 commented Jan 14, 2017

@frrrances Thank you for the links - they are really helpful! It seems like individual page links should also have an aria-label or aria-current attribute, although the article you provided shows a way to assign them using javascript, and kaminari may not necessarily have to have them in the templates. What do you think? Thanks!

@frrrances
Copy link
Contributor Author

@yuki24 Glad they were helpful! It would be nice to have the label and current for the individual pages but not critical. If you're interested, I can see about adding some JS for that as a separate PR, but I like how lean kaminari is :)

@yuki24 yuki24 merged commit 011cd6c into kaminari:master Jan 21, 2017
@yuki24
Copy link
Member

yuki24 commented Jan 21, 2017

@frrrances I've just merged this request in. I think this is a great first step toward adding better accessibility support. I agree that adding javascript would make kaminari less lean, and I think we could just hard-code an aria-label or aria-current attribute in the tag. Thanks again for your contribution!

@frrrances
Copy link
Contributor Author

thanks @yuki24 ! Keep up the awesome work!

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.

None yet

2 participants