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

Improve accessibility #209

merged 5 commits into from Aug 29, 2019


Copy link

commented Aug 28, 2019

See #201

@c-w c-w requested a review from aidan-fitz Aug 28, 2019

@c-w c-w referenced this pull request Aug 28, 2019
7 of 10 tasks complete

This comment has been minimized.

Copy link

commented Aug 28, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #209   +/-   ##
  Coverage   69.52%   69.52%           
  Files          28       28           
  Lines        1693     1693           
  Hits         1177     1177           
  Misses        516      516

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 a04f255...b11fbbc. Read the comment docs.

@c-w c-w force-pushed the accessibility branch from 345b786 to b11fbbc Aug 28, 2019


This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

Merging for now. Let me know if you have any input @aidan-fitz and I'll address it in a future PR.

@c-w c-w merged commit 61d3f8a into master Aug 29, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
continuous-integration/travis-ci/push The Travis CI build passed
Details No dependencies with known security vulnerabilities.

@c-w c-w deleted the accessibility branch Aug 29, 2019

Copy link

left a comment

I like the changes thus far! I have some suggestions for the titles of pages. In my experience, it seems more conventional for page titles to follow the format <name of specific page> | <name of website>, but the mailbox has several subpages that are logically separate from other pages like the homepage and about page, so that would potentially admit a title structure like Write email | Mailbox | Lokole, which is rather clunky.

Also, while I wasn't able to annotate this, you neglected to add <span class="sr-only"> tags before the buttons for ordered and unordered lists.

By the way, can you please summarize your changes in the PR description so it's easier for me to know what to look for in the future? I think this would be especially helpful for PRs that don't address the entire issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
3 participants
You can’t perform that action at this time.