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): aria-current attribute support #3470

Merged

Conversation

peterblazejewicz
Copy link
Contributor

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.

LGTM, thanks!

Will merge when Travis is green


By the way just found out while looking at examples that we're also missing aria-disabled attributes on <a> links → https://getbootstrap.com/docs/4.3/components/pagination/#disabled-and-active-states

If you're motivated :)

@codecov-io
Copy link

Codecov Report

Merging #3470 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3470   +/-   ##
=======================================
  Coverage   91.14%   91.14%           
=======================================
  Files          96       96           
  Lines        2800     2800           
  Branches      516      516           
=======================================
  Hits         2552     2552           
  Misses        190      190           
  Partials       58       58
Flag Coverage Δ
#e2e 56.21% <ø> (ø) ⬆️
#unit 87.99% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/pagination/pagination.ts 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd7aea5...7d2be43. Read the comment docs.

@ihorzenich
Copy link

ihorzenich commented Oct 14, 2020

@peterblazejewicz Keeping <span class="sr-only">(current)</span> together with aria-current="page" on parent cause double announcement of "current" page in NVDA screen-reader.
image

It is necessary to remove <span class="sr-only">(current)</span> from ngb-pagination: #3870

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

4 participants