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

feat(pagination): Added aria label input for accessibility #1294

Closed
wants to merge 1 commit into from

Conversation

thbt
Copy link
Contributor

@thbt thbt commented Feb 6, 2017

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

Hey. Two things:

  1. I would not add ariaLabel on ALL demos, maybe only one like pagination-config or a proper accessibility demo. They are supposed to be simple and to the point.

  2. Maybe I'm talking nonsense, but can't we simply stick [attr.aria-label] on the host element by the way, not on the inner <nav>. They won't read it properly?

@@ -15,4 +15,5 @@ export class NgbPaginationConfig {
pageSize = 10;
rotate = false;
size: 'sm' | 'lg';
ariaLabel: string = 'Page navigation';
Copy link
Member

Choose a reason for hiding this comment

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

no need in :string type

@thbt thbt force-pushed the pagination-a11y branch 2 times, most recently from e0d025d to 63bc0ad Compare February 15, 2017 10:45
@thbt
Copy link
Contributor Author

thbt commented Feb 15, 2017

Thanks for the review!

  1. Makes sense, I removed it from all demo except config.
  2. It might not change a lot for the screen readers, but we wouldn't be in line with bootstrap markup anymore if we applied the attribute on the host instead of the inner <nav>.

(Don't know why the travis build is failing though 🙁 ...)

@thbt thbt closed this Feb 15, 2017
@thbt thbt reopened this Feb 15, 2017
@pkozlowski-opensource
Copy link
Member

Maybe I'm talking nonsense, but can't we simply stick [attr.aria-label] on the host element by the way, not on the inner

. They won't read it properly?

@maxokorokov has a point and tbh I would go even a step further - if we would to simply forward a proerty binding to an attribute on a host... why do we need to do things at all? I mean, why not show people to simply use <ngb-pagination aria-label="...">?

Could we test it with screen readers and check of we need to do anything here at all?

@pkozlowski-opensource
Copy link
Member

I've just checked with ChromeVox and it seems like sticking aria-label on <ngb-pagination> has no effect... So we need to introduce a new input. Was just wondering if we can have it as aria-label instead of ariaLabel (probably not, but worth trying)?

Other thing: this PR doesn't add <span class="sr-only">(current)</span></a> for an active item as done in http://v4-alpha.getbootstrap.com/components/pagination/#disabled-and-active-states - should be added as part of the accessibility work for pagination.

@pkozlowski-opensource
Copy link
Member

@thbt could you please add support for the <span class="sr-only">(current)</span> to this PR as commented above? Ref: http://v4-alpha.getbootstrap.com/components/pagination/#disabled-and-active-states

You might also find this article useful: http://www.a11ymatters.com/pattern/pagination/

@pkozlowski-opensource
Copy link
Member

LGTM. Let's wait for the TravisCI build and if it is green I'm going to merge this one. Thnx!

Remove the `nav` element and add the `role="navigation"` attribute to the
host.
`aria-label` is not needed anymore as it can be put on the host.
Add invisible `(current)` text for screen readers.
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

3 participants