Accessibility improvements: skip link, focus styles, and ARIA labels#18
Accessibility improvements: skip link, focus styles, and ARIA labels#18
Conversation
Changed className from "flex_auto" (invalid) to "flex-auto" (correct Tailwind class) in the Post component's main element. This fixes the flex layout so post pages properly expand to push the footer to the bottom. Closes META-eww
- Added "Skip to main content" link to prose layout - Link is hidden by default (sr-only) and visible on focus - Added id="main" to all main elements in prose pages - Keyboard users can now tab to skip link and bypass navigation Improves accessibility for keyboard navigation. Closes META-02m
- Card component: focus-visible border matching hover state - Likes media links: focus-visible ring for image links - Error page retry button: focus-visible background and ring All interactive elements now have clear keyboard focus indicators. Closes META-sh8, META-pnf, META-86l
There was a problem hiding this comment.
Pull request overview
This PR enhances accessibility across the portfolio site by adding keyboard navigation support, focus indicators, ARIA labels, and comprehensive test coverage. The changes ensure keyboard and screen reader users can effectively navigate and interact with all content. Additionally, pagination positioning is fixed for short blog posts, and several outdated documentation files are removed.
Key changes:
- Added skip link for keyboard users to bypass navigation and jump to main content
- Implemented visible focus states for interactive elements (cards, buttons, links)
- Fixed pagination layout to display above footer on short content pages
- Added comprehensive tests for accessibility features
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/(prose)/layout.tsx | Added skip link element for keyboard navigation |
| app/(prose)/layout.test.tsx | New tests validating skip link functionality |
| app/(prose)/page.tsx | Added id="main" to main element as skip link target |
| app/(prose)/page.test.tsx | Updated test to verify main element has id attribute |
| app/(prose)/blog/page.tsx | Added id="main" to main element |
| app/(prose)/blog/page.test.tsx | Updated test to verify main element has id attribute |
| app/(prose)/not-found.tsx | Added id="main" to main element |
| app/(prose)/[slug]/ui/post.tsx | Fixed flex-auto typo and moved pagination outside article |
| ui/nav/pagination.tsx | Added ARIA label and extracted non-breaking space logic |
| ui/nav/pagination.test.tsx | New comprehensive tests for pagination accessibility |
| ui/card.tsx | Added focus-visible styles for keyboard navigation |
| app/likes/page.tsx | Added focus-visible styles to media item links |
| app/likes/error.tsx | Added focus-visible styles to retry button |
| AGENTS.md | Removed (deprecated documentation) |
| @AGENTS.md | Removed (deprecated documentation) |
| .claude/specs/*.md | Removed multiple outdated spec files |
| .claude/settings.local.json | Simplified configuration |
| .beads/issues.jsonl | Updated issue tracking data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </article> | ||
| </main> | ||
| <PaginationLinks prevPost={prevPost} nextPost={nextPost} /> |
There was a problem hiding this comment.
Moving pagination outside the article element changes the semantic structure - pagination is now a sibling of main rather than a child. Consider whether this affects screen reader navigation and document outline. If pagination is conceptually part of the post content navigation, it might be better to keep it inside the article but outside the main element, using a wrapping fragment around both main and nav.
| </article> | |
| </main> | |
| <PaginationLinks prevPost={prevPost} nextPost={nextPost} /> | |
| <PaginationLinks prevPost={prevPost} nextPost={nextPost} /> | |
| </article> | |
| </main> |
| const NON_BREAKING_SPACE = '\u00A0' | ||
|
|
||
| /** | ||
| * Replaces the last space in a string with a non-breaking space. | ||
| * This prevents awkward line wrapping where the last word appears alone on a new line. | ||
| * | ||
| * @example | ||
| * replaceLastSpaceWithNonBreaking("Hello World") // "Hello\u00A0World" | ||
| */ | ||
| export function replaceLastSpaceWithNonBreaking(text: string): string { | ||
| return text.replace(/ (?!.* )/, NON_BREAKING_SPACE) | ||
| } |
There was a problem hiding this comment.
The JSDoc example shows the Unicode escape sequence in the comment, but actual usage will show as a regular space character. Consider updating the example to clarify what the visual output looks like: // Returns \"Hello World\" (with non-breaking space before \"World\") to make it clearer that there's no visible difference.
Pull Request Review: Accessibility ImprovementsOverviewThis PR makes important accessibility improvements to the site, adding keyboard navigation support, focus styles, and ARIA labels. The changes are well-implemented with comprehensive test coverage. ✅ Strengths1. Excellent Test Coverage
2. Proper Accessibility Implementation
3. Clean, Maintainable Code
4. Good Separation of Concerns
🔍 Areas for Improvement1. Security: Link Target Safety 2. Code Quality: Emoji Direction Logic 3. Accessibility: Focus Ring Color Contrast 4. Test Coverage: Missing Edge Cases 5. Performance: CSS Class String Concatenation 📋 Checklist
🎯 RecommendationsHigh Priority:
Medium Priority: Low Priority: 🚀 Overall AssessmentLGTM with minor suggestions. This PR makes meaningful accessibility improvements with strong test coverage. The implementation follows modern best practices and integrates cleanly with the existing codebase. The suggestions above are mostly minor enhancements rather than blockers. The unrelated cleanup (deletion of Claude markdown files) is noted in the PR description and does not affect code quality. Score: 8.5/10
Great work! 🎉 |
✅ What
beads-oriented workflow🤔 Why