feat: add floating docs navigation shortcuts #4632
Conversation
Yash-kurosaki
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughA new DocsNavigationShortcuts component was added to provide keyboard shortcuts (arrow keys) and persistent UI buttons for navigating between documentation pages. The component was integrated into DocsLayout and renders conditionally based on viewport size. Changes
Sequence DiagramsequenceDiagram
participant User
participant Component as DocsNavigationShortcuts
participant DOM as Event Listener
participant Router as Next.js Router
User->>DOM: Presses Arrow Key / Clicks Button
DOM->>Component: onKeyDown / onClick
Component->>Component: Check if focus on<br/>editable/interactive element
alt Not Editing
Component->>Component: Determine prevPage/<br/>nextPage target
Component->>Router: Navigate via Link/Router
Router->>User: Load next/prev doc page
else Editing
Component->>User: Ignore (suppress navigation)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Areas to verify:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4632--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/layout/DocsLayout.tsx (1)
1-24: Fix import ordering persimple-import-sortto satisfy lintThe GitHub Actions run is failing with
simple-import-sort/importson Line 1. This is triggered by the newDocsNavigationShortcutsimport disrupting the expected ordering/grouping.Rather than hand-tuning the order, it’s safest to run the project’s autofix:
npm run lint -- --fix(or the equivalent lint command for this repo)
This will reorder the imports according to the configured
simple-import-sortrules and unblock CI.
🧹 Nitpick comments (2)
components/navigation/DocsNavigationShortcuts.tsx (2)
14-40: Optional: broaden the “interactive” guard to include focused linksThe global ArrowLeft/ArrowRight handler correctly skips most interactive elements via
INTERACTIVE_TAGSandisContentEditable. However, when focus is on a link (<a>rendered bynext/link), arrow keys will still trigger page navigation.If you’d prefer not to override arrow behavior when a link itself is focused, consider including
'a'inINTERACTIVE_TAGS:-const INTERACTIVE_TAGS = new Set(['input', 'textarea', 'select', 'button']); +const INTERACTIVE_TAGS = new Set(['input', 'textarea', 'select', 'button', 'a']);This is mostly a UX choice; current behavior is reasonable, but this tweak avoids surprises for keyboard users who are focused on a link.
16-30: Add brief JSDoc for the new component (and optionally the handler) to satisfy lint warningsThe pipeline is flagging missing JSDoc on the exported component (Line 16) and the inner handler (Line 30). A minimal description is enough to satisfy the rule and document behavior:
+/** + * Floating previous/next docs navigation with global ArrowLeft/ArrowRight shortcuts. + */ export default function DocsNavigationShortcuts({ post }: IDocsNavigationShortcutsProps) { @@ - function handleKeyDown(event: KeyboardEvent) { + /** + * Handles global ArrowLeft/ArrowRight presses to navigate between docs pages. + */ + function handleKeyDown(event: KeyboardEvent) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/layout/DocsLayout.tsx(2 hunks)components/navigation/DocsNavigationShortcuts.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/layout/DocsLayout.tsxcomponents/navigation/DocsNavigationShortcuts.tsx
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/layout/DocsLayout.tsxcomponents/navigation/DocsNavigationShortcuts.tsx
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
components/navigation/DocsNavigationShortcuts.tsx
🧬 Code graph analysis (2)
components/layout/DocsLayout.tsx (1)
components/navigation/DocsNavigationShortcuts.tsx (1)
DocsNavigationShortcuts(16-89)
components/navigation/DocsNavigationShortcuts.tsx (1)
components/icons/ArrowLeft.tsx (1)
ArrowLeft(7-17)
🪛 GitHub Actions: PR testing - if Node project
components/layout/DocsLayout.tsx
[error] 1-1: Run autofix to sort these imports! simple-import-sort/imports
components/navigation/DocsNavigationShortcuts.tsx
[warning] 16-16: Missing JSDoc comment.
[error] 22-22: Prettier formatting: Delete , and Unexpected trailing comma.
[error] 24-24: Prettier formatting: Delete , and Unexpected trailing comma.
[warning] 30-30: Missing JSDoc comment.
[warning] 51-51: Padding and formatting rule: Expected blank line before this statement.
[warning] 51-51: Consistent return: Arrow function should return a value or explicitly return null.
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lighthouse CI
🔇 Additional comments (3)
components/navigation/DocsNavigationShortcuts.tsx (2)
54-88: Floating nav UI and accessibility look solidThe floating prev/next buttons are well integrated: they’re hidden on small screens (
hidden md:flex), usepointer-events-noneon the container withpointer-events-autoon the buttons, and expose cleararia-labels plus visible titles/labels for different breakpoints. This should provide a good UX with minimal overlap/interaction issues.
19-25: Fix Prettier trailing commas inuseMemoto unblock CIThe pipeline is failing on the trailing commas in the
useMemoblock (object literal and dependency array). Adjusting them to match the repo’s Prettier config should clear the errors.const navigationTargets = useMemo( () => ({ prev: post?.prevPage?.href, - next: post?.nextPage?.href, + next: post?.nextPage?.href }), - [post?.prevPage?.href, post?.nextPage?.href], + [post?.prevPage?.href, post?.nextPage?.href] );⛔ Skipped due to learnings
Learnt from: akshatnema Repo: asyncapi/website PR: 3262 File: components/Avatar.tsx:35-44 Timestamp: 2024-10-11T10:46:58.882Z Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.Learnt from: akshatnema Repo: asyncapi/website PR: 3262 File: components/newsroom/FeaturedBlogPost.tsx:90-92 Timestamp: 2024-10-11T07:38:35.745Z Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.Learnt from: akshatnema Repo: asyncapi/website PR: 3262 File: components/navigation/BlogPostItem.tsx:95-119 Timestamp: 2024-10-11T11:32:30.226Z Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.components/layout/DocsLayout.tsx (1)
121-131: Good integration point forDocsNavigationShortcutsin the layoutPlacing
<DocsNavigationShortcuts post={post} />at the top of the<main>(which is alreadyposition: relative) is a good choice: the component itself uses a fixed, centered overlay and handles visibility by breakpoint, so it doesn’t interfere with the existing mobile menu or TOC layout. Passingpostdirectly keeps prev/next resolution aligned with the rest of the docs UI.
| useEffect(() => { | ||
| if (!navigationTargets.prev && !navigationTargets.next) return; | ||
|
|
||
| function handleKeyDown(event: KeyboardEvent) { | ||
| if (event.defaultPrevented) return; | ||
| if (event.metaKey || event.ctrlKey || event.altKey) return; | ||
| if (event.key !== 'ArrowLeft' && event.key !== 'ArrowRight') return; | ||
|
|
||
| const target = event.target as HTMLElement | null; | ||
| const tagName = target?.tagName?.toLowerCase(); | ||
| const isEditable = target?.isContentEditable; | ||
|
|
||
| if (isEditable || (tagName && INTERACTIVE_TAGS.has(tagName))) return; | ||
|
|
||
| if (event.key === 'ArrowLeft' && navigationTargets.prev) { | ||
| router.push(navigationTargets.prev); | ||
| event.preventDefault(); | ||
| } else if (event.key === 'ArrowRight' && navigationTargets.next) { | ||
| router.push(navigationTargets.next); | ||
| event.preventDefault(); | ||
| } | ||
| } | ||
|
|
||
| window.addEventListener('keydown', handleKeyDown); | ||
| return () => window.removeEventListener('keydown', handleKeyDown); | ||
| }, [navigationTargets, router]); |
There was a problem hiding this comment.
Refine useEffect for consistent-return + padding rules and clearer cleanup
The early return plus returning a cleanup function in another path is triggering the consistent-return warning, and the linter also wants a blank line before the cleanup return. You can address both and make the cleanup more explicit like this:
useEffect(() => {
- if (!navigationTargets.prev && !navigationTargets.next) return;
+ if (!navigationTargets.prev && !navigationTargets.next) {
+ return undefined;
+ }
@@
- window.addEventListener('keydown', handleKeyDown);
- return () => window.removeEventListener('keydown', handleKeyDown);
+ window.addEventListener('keydown', handleKeyDown);
+
+ return () => {
+ window.removeEventListener('keydown', handleKeyDown);
+ };
}, [navigationTargets, router]);This keeps behavior the same while satisfying the lint rules and making the lifecycle a bit clearer.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (!navigationTargets.prev && !navigationTargets.next) return; | |
| function handleKeyDown(event: KeyboardEvent) { | |
| if (event.defaultPrevented) return; | |
| if (event.metaKey || event.ctrlKey || event.altKey) return; | |
| if (event.key !== 'ArrowLeft' && event.key !== 'ArrowRight') return; | |
| const target = event.target as HTMLElement | null; | |
| const tagName = target?.tagName?.toLowerCase(); | |
| const isEditable = target?.isContentEditable; | |
| if (isEditable || (tagName && INTERACTIVE_TAGS.has(tagName))) return; | |
| if (event.key === 'ArrowLeft' && navigationTargets.prev) { | |
| router.push(navigationTargets.prev); | |
| event.preventDefault(); | |
| } else if (event.key === 'ArrowRight' && navigationTargets.next) { | |
| router.push(navigationTargets.next); | |
| event.preventDefault(); | |
| } | |
| } | |
| window.addEventListener('keydown', handleKeyDown); | |
| return () => window.removeEventListener('keydown', handleKeyDown); | |
| }, [navigationTargets, router]); | |
| useEffect(() => { | |
| if (!navigationTargets.prev && !navigationTargets.next) { | |
| return undefined; | |
| } | |
| function handleKeyDown(event: KeyboardEvent) { | |
| if (event.defaultPrevented) return; | |
| if (event.metaKey || event.ctrlKey || event.altKey) return; | |
| if (event.key !== 'ArrowLeft' && event.key !== 'ArrowRight') return; | |
| const target = event.target as HTMLElement | null; | |
| const tagName = target?.tagName?.toLowerCase(); | |
| const isEditable = target?.isContentEditable; | |
| if (isEditable || (tagName && INTERACTIVE_TAGS.has(tagName))) return; | |
| if (event.key === 'ArrowLeft' && navigationTargets.prev) { | |
| router.push(navigationTargets.prev); | |
| event.preventDefault(); | |
| } else if (event.key === 'ArrowRight' && navigationTargets.next) { | |
| router.push(navigationTargets.next); | |
| event.preventDefault(); | |
| } | |
| } | |
| window.addEventListener('keydown', handleKeyDown); | |
| return () => { | |
| window.removeEventListener('keydown', handleKeyDown); | |
| }; | |
| }, [navigationTargets, router]); |
🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[warning] 30-30: Missing JSDoc comment.
[warning] 51-51: Padding and formatting rule: Expected blank line before this statement.
[warning] 51-51: Consistent return: Arrow function should return a value or explicitly return null.
🤖 Prompt for AI Agents
In components/navigation/DocsNavigationShortcuts.tsx around lines 27 to 52, the
effect currently early-returns in one path and returns a cleanup function in
another which trips the consistent-return rule and the linter wants a blank line
before the cleanup return; change the early exit to return an explicit no-op
cleanup (e.g., return () => {}), add a blank line immediately before the final
return that removes the event listener, and keep the rest of the handler logic
intact so the effect always returns a function and the cleanup is clearly
separated.
|
I would vote against having floating buttons that overlap with the main content. This is kinda awful UX. |
3 participants
This PR implements the floating docs navigation shortcuts as requested in issue #4629.
Summary
DocsNavigationShortcutscomponentDocsLayout.tsxnpm run devhere is the screenshot :
Issue Reference
Fixes #4629
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.