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: add aria-labels to nav elements #358

Merged
merged 7 commits into from
Dec 12, 2023

Conversation

CBID2
Copy link
Contributor

@CBID2 CBID2 commented Nov 17, 2023

What does this PR do?

This PR adds an aria-label to <nav> elements. This to make the site more accessible for users who use screen readers to navigate the site.

Test Plan

To test this, I plan on using an accessiblity chrome extension to see if it conforms to accessibility standards.

Related PRs and Issues

Closes #294

Have you read the Contributing Guidelines on issues?

Yes

Copy link

vercel bot commented Nov 17, 2023

@CBID2 is attempting to deploy a commit to the appwrite Team on Vercel.

A member of the Team first needs to authorize it.

@CBID2 CBID2 marked this pull request as ready for review November 17, 2023 22:13
@CBID2 CBID2 marked this pull request as draft November 17, 2023 22:22
src/lib/components/FooterNav.svelte Outdated Show resolved Hide resolved
src/lib/layouts/Docs.svelte Outdated Show resolved Hide resolved
src/lib/layouts/Main.svelte Outdated Show resolved Hide resolved
src/lib/layouts/Sidebar.svelte Outdated Show resolved Hide resolved
src/markdoc/tags/Tabs.svelte Outdated Show resolved Hide resolved
@TGlide
Copy link
Contributor

TGlide commented Nov 21, 2023

@CBID2 Thank you for the PR!! I left a few comments 🙂

@CBID2 CBID2 marked this pull request as ready for review November 21, 2023 19:10
@CBID2
Copy link
Contributor Author

CBID2 commented Nov 21, 2023

Hi @TGlide! :) I made the changes you suggested! :)

@@ -26,7 +26,6 @@
</script>

<div class="aw-card is-normal u-margin-block-start-16" {...$root} use:root>
<nav class="tabs u-flex u-gap-16">
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, my wording was off here. By removing the nav element, I meant swapping it with something else, e.g. a div. Otherwise we lose the styling we had here.

@CBID2
Copy link
Contributor Author

CBID2 commented Nov 29, 2023

Hi @TGlide! :) I made the changes! :)

Copy link
Contributor

@TGlide TGlide left a comment

Choose a reason for hiding this comment

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

Awesome 😄

@TGlide TGlide merged commit e1724f8 into appwrite:main Dec 12, 2023
1 of 2 checks passed
@CBID2 CBID2 deleted the making-nav-more-accessible branch December 12, 2023 20:51
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.

📚 Documentation: multiple <nav> elements should be labelled [a11y][TheA11y100]
2 participants