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

Remove <nav> from pagination #686

Merged
merged 2 commits into from
Sep 19, 2024
Merged

Remove <nav> from pagination #686

merged 2 commits into from
Sep 19, 2024

Conversation

xi
Copy link
Contributor

@xi xi commented Sep 16, 2024

fixes #449

Providing a fixed label is not feasible because this project does not have any translations yet.

From the other two options, I guess that not having a <nav> element is better than having an unusable one.

fixes zostera#449

Providing a fixed label is not feasible because this project does not
have any translations yet.

From the other two options, I guess that not having a <nav> element is
better than having an unusable one.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 10888063486

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.944%

Totals Coverage Status
Change from base Build 10869645241: 0.0%
Covered Lines: 1368
Relevant Lines: 1389

💛 - Coveralls

@dyve
Copy link
Member

dyve commented Sep 17, 2024

Removing the nav would step away from the best practice in Bootstrap examples that we follow.

@dyve
Copy link
Member

dyve commented Sep 17, 2024

Perhaps a setting for pagination_aria_label?

@xi
Copy link
Contributor Author

xi commented Sep 17, 2024

Removing the nav would step away from the best practice in Bootstrap examples that we follow.

Not really. As explained in #449, the current implementation does not follow best practices because it does not contain a label. Removing the <nav> here and telling authors to add it themselves is the proper fix IMHO.

Adding a parameter would allow authors to fix this issue, but the default would still be broken. That's why it is not my preferred option. But if you really think that adding a parameter is the way to go, I can adapt the code accordingly.

@dyve
Copy link
Member

dyve commented Sep 18, 2024

I don't use screen readers myself. I can imagine that a nav without context is confusing, but that's about it. It would be really helpful to get feedback from someone that actually needs or benefits from this change.

@xi
Copy link
Contributor Author

xi commented Sep 18, 2024

I sent a mail to the Web Accessibility Initiative Interest Group Discussion list: https://lists.w3.org/Archives/Public/w3c-wai-ig/2024JulSep/0113.html

So far, both answers recommended to remove the <nav> element. Not just because of the missing label, but also because the question whether the pagination should be a landmark should be considered in the context of the whole page. For example, it would be bad if authors do not add any landmarks themselves and the pagination ends up being the only landmark on the page.

@dyve
Copy link
Member

dyve commented Sep 19, 2024

I'd say that settles it then. And we have documented the why and what in this issue. Thanks for all the effort you put into this @xi 🙏

@dyve dyve merged commit 0b0c70f into zostera:main Sep 19, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing aria-label on pagination <nav> element
3 participants