feat(Tour): feat: support keyboard operation #87
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough本次变更为 Tour 组件新增可选 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as 用户
participant Win as Window
participant Tour as Tour 组件
participant State as 状态(current, open, steps)
participant Host as onClose 回调
User->>Win: 键盘按键 (← / → / Esc)
Win->>Tour: keydown 事件（仅在 keyboard = true 时）
alt ArrowLeft
Tour->>State: current = max(0, current - 1)
Tour-->>User: 阻止默认/冒泡
else ArrowRight
Tour->>State: current = min(last, current + 1)
Tour-->>User: 阻止默认/冒泡
else Escape
Tour->>State: open = false
Tour->>Host: onClose(current)
Tour-->>User: 阻止默认/冒泡
end
note right of Tour: 全局监听在组件挂载/更新时注册，卸载时清理
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
Summary of ChangesHello @Linkodt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the accessibility and usability of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces keyboard navigation for the Tour component, which is a great feature enhancement. The implementation is mostly solid, including the addition of a new demo page. However, I've identified a potential bug in the keyboard event handler related to stale closures due to missing dependencies in a useCallback. I've provided a detailed comment with a suggested fix that also improves code readability. Additionally, I've noted a minor design consideration regarding the use of a global event listener that could be improved for more complex use cases.
src/Tour.tsx
Outdated
| const handleKeyDown = React.useCallback((e: KeyboardEvent) => { | ||
| if (!mergedOpen) { | ||
| return; | ||
| } | ||
| if (e.key === 'ArrowLeft') { | ||
| if (mergedCurrent <= 0) { | ||
| return; | ||
| } | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onInternalChange(mergedCurrent - 1); | ||
| return; | ||
| } | ||
| if (e.key === 'ArrowRight') { | ||
| if (mergedCurrent >= steps.length - 1) { | ||
| return; | ||
| } | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onInternalChange(mergedCurrent + 1); | ||
| return; | ||
| } | ||
| if (e.key === 'Escape') { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| handleClose(); | ||
| return; | ||
| } | ||
| }, [mergedCurrent, mergedOpen, steps.length]); |
There was a problem hiding this comment.
The implementation of handleKeyDown has a potential bug and can be improved for readability.
-
Stale Closures: The
useCallbackhook forhandleKeyDownis missingonInternalChangeandhandleClosein its dependency array. This can lead to bugs where stale versions of these functions are called if theonChangeoronCloseprops change. Adding them to the dependency array is necessary for correctness. -
Readability: The chain of
ifstatements for key handling can be refactored into aswitchstatement. This improves readability and maintainability.
Important Note on Performance: Adding the missing dependencies will fix the bug, but since onInternalChange and handleClose are not memoized, handleKeyDown will be recreated on every render. This will cause the useEffect at line 198 to remove and re-add the global event listener on every render, which can be a performance concern. The ideal solution would be to also wrap onInternalChange and handleClose in useCallback where they are defined.
const handleKeyDown = React.useCallback((e: KeyboardEvent) => {
if (!mergedOpen) {
return;
}
switch (e.key) {
case 'ArrowLeft':
if (mergedCurrent > 0) {
e.preventDefault();
e.stopPropagation();
onInternalChange(mergedCurrent - 1);
}
break;
case 'ArrowRight':
if (mergedCurrent < steps.length - 1) {
e.preventDefault();
e.stopPropagation();
onInternalChange(mergedCurrent + 1);
}
break;
case 'Escape':
e.preventDefault();
e.stopPropagation();
handleClose();
break;
default:
// Do nothing for other keys
break;
}
}, [mergedCurrent, mergedOpen, steps.length, onInternalChange, handleClose]);
| React.useEffect(() => { | ||
| if (keyboard) { | ||
| document.addEventListener('keydown', handleKeyDown); | ||
| } | ||
| return () => { | ||
| document.removeEventListener('keydown', handleKeyDown); | ||
| }; | ||
| }, [handleKeyDown, keyboard]); |
There was a problem hiding this comment.
Attaching the event listener directly to document can lead to issues if multiple Tour components are rendered and open at the same time, as they would all react to the keyboard events. While it's unlikely to have multiple tours open simultaneously, a more robust implementation might involve a mechanism to ensure only the top-most or most recently interacted-with tour handles the keyboard events. This is a common pattern in component libraries for managing global listeners for modals and dialogs. This is something to consider for future improvements to prevent potential conflicts in more complex applications.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/interface.ts (1)
92-92: 建议添加 JSDoc 注释以改进 API 文档。虽然类型定义正确，但建议为这个新的公共 API 属性添加 JSDoc 注释，说明其用途、默认值和行为。这将改善开发者体验和 IDE 自动完成提示。
应用此差异添加文档注释：
+ /** Enable keyboard navigation (ArrowLeft/Right for steps, Escape to close). @default false */ keyboard?: boolean;src/Tour.tsx (3)
168-196: 建议检查修饰键以改善用户体验。当前实现会拦截所有 ArrowLeft/Right 按键，这可能干扰用户在输入框中使用 Ctrl+Arrow 进行单词导航等操作。建议仅在没有修饰键时处理方向键。
应用此差异添加修饰键检查：
const handleKeyDown = React.useCallback((e: KeyboardEvent) => { if (!mergedOpen) { return; } + // 忽略带有修饰键的按键事件 + if (e.ctrlKey || e.metaKey || e.altKey || e.shiftKey) { + return; + } if (e.key === 'ArrowLeft') {
198-205: 频繁的监听器重新注册可能影响性能。由于
handleKeyDown依赖于频繁变化的状态（mergedCurrent），useEffect 会在每次步骤切换时移除并重新添加监听器。虽然功能正常，但可以通过使用 ref 存储最新状态来优化。优化方案：
+ const stateRef = React.useRef({ mergedCurrent, mergedOpen, stepsLength: steps.length }); + + React.useEffect(() => { + stateRef.current = { mergedCurrent, mergedOpen, stepsLength: steps.length }; + }, [mergedCurrent, mergedOpen, steps.length]); + - const handleKeyDown = React.useCallback((e: KeyboardEvent) => { - if (!mergedOpen) { + const handleKeyDown = React.useCallback(() => { + const { mergedOpen, mergedCurrent, stepsLength } = stateRef.current; + if (!mergedOpen) { return; } // ... 其余逻辑使用 ref 中的值 - }, [mergedCurrent, mergedOpen, steps.length, handleClose, onInternalChange]); + }, [handleClose, onInternalChange]);这样监听器只在
keyboardprop 变化时重新注册。
61-205: 建议增强键盘导航的可访问性。当前实现功能正常，但可以通过以下方式改善可访问性和用户体验：
- 焦点管理：Tour 打开时，考虑将焦点移至 Tour 内容，关闭时恢复焦点到之前的元素
- 视觉提示：在 Tour 界面上添加键盘快捷键提示（如 "使用 ← → 导航，ESC 关闭"）
- 焦点陷阱：当
keyboard={true}时，考虑实现焦点陷阱（focus trap），防止 Tab 键跳出 Tour 区域- ARIA 属性：添加
aria-keyshortcuts属性说明可用的键盘快捷键示例：在 Tour 内容中添加键盘提示：
// 在 TourStep 组件中添加 <div className={`${prefixCls}-keyboard-hint`} aria-live="polite"> 使用 ← → 键导航，按 ESC 关闭 </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/demo/keyboard.md(1 hunks)docs/examples/keyboard.tsx(1 hunks)src/Tour.tsx(2 hunks)src/interface.ts(1 hunks)
🔇 Additional comments (4)
docs/demo/keyboard.md (1)
1-8: 文档结构正确！文档格式符合规范，正确引用了示例代码。
src/Tour.tsx (3)
61-61: 默认值设置合理！将
keyboard默认设为false是明智的选择，采用渐进增强的方式，不会影响现有用户。
162-165: handleClose 实现正确！函数正确地关闭 Tour 并通知父组件当前索引，逻辑清晰。
228-228: 正确使用 handleClose！将
handleClose传递给TourStep的onCloseprop 是正确的，确保了关闭按钮和 Escape 键行为的一致性。
| <Tour | ||
| defaultCurrent={0} | ||
| keyboard={true} | ||
| steps={[ | ||
| { | ||
| title: 'One', | ||
| target: () => firstBtnRef.current, | ||
| }, | ||
| { | ||
| title: 'Two', | ||
| target: () => secondBtnRef.current, | ||
| }, | ||
| { | ||
| title: 'Three', | ||
| target: () => thirdBtnRef.current, | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
示例缺少 Tour 可见性控制。
当前示例中 Tour 组件始终处于打开状态（依赖 defaultCurrent 但没有 open prop 控制）。这可能让用户难以理解如何在实际应用中初始打开和关闭 Tour。
建议添加状态控制和触发按钮来演示完整的使用场景：
+import React, { useRef, useState } from 'react';
-import React, { useRef } from 'react';
import Tour from '../../src/index';
import './basic.less';
const App = () => {
+ const [open, setOpen] = useState(false);
const firstBtnRef = useRef<HTMLButtonElement>(null);
const secondBtnRef = useRef<HTMLButtonElement>(null);
const thirdBtnRef = useRef<HTMLButtonElement>(null);
return (
<React.StrictMode>
<div style={{ margin: 20 }}>
+ <button onClick={() => setOpen(true)}>开始导览</button>
<div>
<button
className="ant-target"
ref={firstBtnRef}
>
One
</button>
{/* ... */}
</div>
<Tour
+ open={open}
+ onClose={() => setOpen(false)}
defaultCurrent={0}
keyboard={true}
steps={[
{/* ... */}
]}
/>
</div>
</React.StrictMode>
);
};📝 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.
| <Tour | |
| defaultCurrent={0} | |
| keyboard={true} | |
| steps={[ | |
| { | |
| title: 'One', | |
| target: () => firstBtnRef.current, | |
| }, | |
| { | |
| title: 'Two', | |
| target: () => secondBtnRef.current, | |
| }, | |
| { | |
| title: 'Three', | |
| target: () => thirdBtnRef.current, | |
| }, | |
| ]} | |
| /> | |
| import React, { useRef, useState } from 'react'; | |
| import Tour from '../../src/index'; | |
| import './basic.less'; | |
| const App = () => { | |
| const [open, setOpen] = useState(false); | |
| const firstBtnRef = useRef<HTMLButtonElement>(null); | |
| const secondBtnRef = useRef<HTMLButtonElement>(null); | |
| const thirdBtnRef = useRef<HTMLButtonElement>(null); | |
| return ( | |
| <React.StrictMode> | |
| <div style={{ margin: 20 }}> | |
| <button onClick={() => setOpen(true)}>开始导览</button> | |
| <div> | |
| <button | |
| className="ant-target" | |
| ref={firstBtnRef} | |
| > | |
| One | |
| </button> | |
| {/* ...other buttons... */} | |
| </div> | |
| <Tour | |
| open={open} | |
| onClose={() => setOpen(false)} | |
| defaultCurrent={0} | |
| keyboard={true} | |
| steps={[ | |
| { | |
| title: 'One', | |
| target: () => firstBtnRef.current, | |
| }, | |
| { | |
| title: 'Two', | |
| target: () => secondBtnRef.current, | |
| }, | |
| { | |
| title: 'Three', | |
| target: () => thirdBtnRef.current, | |
| }, | |
| ]} | |
| /> | |
| </div> | |
| </React.StrictMode> | |
| ); | |
| }; | |
| export default App; |
🤖 Prompt for AI Agents
In docs/examples/keyboard.tsx around lines 29 to 46, the Tour is always shown
because it only uses defaultCurrent and lacks open state control; add a boolean
state (e.g., const [open, setOpen] = useState(false)) and pass open={open} and
onClose={() => setOpen(false)} to the Tour, add a trigger button that calls
setOpen(true) to open the tour, and ensure any initial step control still uses
defaultCurrent or useCurrent as needed so the example demonstrates opening,
closing, and triggering the Tour in a real app.
| React.useEffect(() => { | ||
| if (keyboard) { | ||
| document.addEventListener('keydown', handleKeyDown); | ||
| } | ||
| return () => { | ||
| document.removeEventListener('keydown', handleKeyDown); | ||
| }; | ||
| }, [handleKeyDown, keyboard]); |
There was a problem hiding this comment.
多个 Tour 实例可能导致键盘事件冲突。
如果页面上同时挂载多个启用 keyboard 的 Tour 实例，所有实例的监听器都会触发，可能导致意外的行为或冲突。建议添加逻辑确保只有当前活动的 Tour 实例处理键盘事件。
可能的解决方案：
- 使用事件目标检查或 z-index 比较确定哪个 Tour 应该处理事件
- 使用全局状态管理确保只有一个 Tour 监听键盘事件
- 在文档中明确说明不支持多个同时打开的 Tour 使用键盘导航
示例修复（检查 Tour 是否打开）：
const handleKeyDown = React.useCallback((e: KeyboardEvent) => {
- if (!mergedOpen) {
+ if (!mergedOpen || !keyboard) {
return;
}并在文档中说明此限制。
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Tour.tsx around lines 198-205, multiple mounted Tour instances that
enable keyboard can all add the same keydown listener causing conflicts; modify
the effect to only attach the listener when this Tour is the active/open one
(e.g., check isOpen/active state or an activeTourId from context/global store)
and include that same condition in the cleanup so only the active instance
handles key events; alternatively register a single global keyboard handler that
routes events to the active Tour by id; update docs to state that keyboard
navigation only applies to the active/open Tour instance.
|
@Linkodt is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Tour.tsx (3)
161-165: 考虑使用 useCallback 包裹以优化性能。
handleClose函数逻辑正确，但未使用useCallback包裹，这会导致每次渲染时创建新的函数引用。由于handleKeyDown依赖此函数，这会导致键盘监听器在每次渲染时重新注册，可能影响性能。应用此差异来包裹函数：
- const handleClose = () => { + const handleClose = React.useCallback(() => { setMergedOpen(false); onClose?.(mergedCurrent); - }; + }, [mergedCurrent, onClose]);
173-199: 可以使用 switch 语句提高可读性。当前使用多个 if 语句处理不同按键。使用 switch 语句可以使代码结构更清晰、更易维护。
应用此差异重构为 switch 语句：
- if (e.key === 'ArrowLeft') { + switch (e.key) { + case 'ArrowLeft': - if (mergedCurrent <= 0) { - return; - } - e.preventDefault(); - e.stopPropagation(); - onInternalChange(mergedCurrent - 1); - return; - } - if (e.key === 'ArrowRight') { - if (mergedCurrent >= steps.length - 1) { - return; - } - e.preventDefault(); - e.stopPropagation(); - onInternalChange(mergedCurrent + 1); - return; - } - if (e.key === 'Escape') { - if (!mergedClosable) { - return; - } - e.preventDefault(); - e.stopPropagation(); - handleClose(); - return; + if (mergedCurrent > 0) { + e.preventDefault(); + e.stopPropagation(); + onInternalChange(mergedCurrent - 1); + } + break; + case 'ArrowRight': + if (mergedCurrent < steps.length - 1) { + e.preventDefault(); + e.stopPropagation(); + onInternalChange(mergedCurrent + 1); + } + break; + case 'Escape': + if (mergedClosable) { + e.preventDefault(); + e.stopPropagation(); + handleClose(); + } + break; + default: + // 其他按键不处理 + break; }
168-168: 优化 AbortController 的初始化。第 168 行初始化的 AbortController 在第一次 effect 运行时立即被中止（第 203 行），从未被实际使用。可以将其初始化为
null以避免创建不必要的对象。应用此差异优化初始化：
- const keyboardController = React.useRef(new AbortController()); + const keyboardController = React.useRef<AbortController | null>(null);然后在 effect 中添加 null 检查：
React.useEffect(() => { - keyboardController.current.abort(); + if (keyboardController.current) { + keyboardController.current.abort(); + } keyboardController.current = new AbortController();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Tour.tsx(2 hunks)
🔇 Additional comments (2)
src/Tour.tsx (2)
61-61: LGTM!
keyboard属性的添加方式正确，默认值为false保证了向后兼容性。
202-213: 事件监听器设置正确。使用 AbortController 管理事件监听器的模式是正确的，能够确保在组件卸载或依赖变化时正确清理监听器。
| const handleKeyDown = React.useCallback((e: KeyboardEvent) => { | ||
| if (!mergedOpen) { | ||
| return; | ||
| } | ||
| if (e.key === 'ArrowLeft') { | ||
| if (mergedCurrent <= 0) { | ||
| return; | ||
| } | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onInternalChange(mergedCurrent - 1); | ||
| return; | ||
| } | ||
| if (e.key === 'ArrowRight') { | ||
| if (mergedCurrent >= steps.length - 1) { | ||
| return; | ||
| } | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onInternalChange(mergedCurrent + 1); | ||
| return; | ||
| } | ||
| if (e.key === 'Escape') { | ||
| if (!mergedClosable) { | ||
| return; | ||
| } | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| handleClose(); | ||
| return; | ||
| } | ||
| }, [mergedCurrent, mergedOpen, steps.length, handleClose, onInternalChange]); |
There was a problem hiding this comment.
缺少 mergedClosable 依赖导致闭包陈旧。
handleKeyDown 在第 192 行使用了 mergedClosable，但该变量未包含在依赖数组中。这会导致回调捕获过时的 mergedClosable 值，当可关闭状态变化时，Escape 键行为不会更新。
应用此差异修复依赖数组：
- }, [mergedCurrent, mergedOpen, steps.length, handleClose, onInternalChange]);
+ }, [mergedCurrent, mergedOpen, steps.length, handleClose, onInternalChange, mergedClosable]);📝 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.
| const handleKeyDown = React.useCallback((e: KeyboardEvent) => { | |
| if (!mergedOpen) { | |
| return; | |
| } | |
| if (e.key === 'ArrowLeft') { | |
| if (mergedCurrent <= 0) { | |
| return; | |
| } | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| onInternalChange(mergedCurrent - 1); | |
| return; | |
| } | |
| if (e.key === 'ArrowRight') { | |
| if (mergedCurrent >= steps.length - 1) { | |
| return; | |
| } | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| onInternalChange(mergedCurrent + 1); | |
| return; | |
| } | |
| if (e.key === 'Escape') { | |
| if (!mergedClosable) { | |
| return; | |
| } | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| handleClose(); | |
| return; | |
| } | |
| }, [mergedCurrent, mergedOpen, steps.length, handleClose, onInternalChange]); | |
| const handleKeyDown = React.useCallback((e: KeyboardEvent) => { | |
| if (!mergedOpen) { | |
| return; | |
| } | |
| if (e.key === 'ArrowLeft') { | |
| if (mergedCurrent <= 0) { | |
| return; | |
| } | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| onInternalChange(mergedCurrent - 1); | |
| return; | |
| } | |
| if (e.key === 'ArrowRight') { | |
| if (mergedCurrent >= steps.length - 1) { | |
| return; | |
| } | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| onInternalChange(mergedCurrent + 1); | |
| return; | |
| } | |
| if (e.key === 'Escape') { | |
| if (!mergedClosable) { | |
| return; | |
| } | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| handleClose(); | |
| return; | |
| } | |
| }, [mergedCurrent, mergedOpen, steps.length, handleClose, onInternalChange, mergedClosable]); |
🤖 Prompt for AI Agents
In src/Tour.tsx around lines 169 to 200, the handleKeyDown callback uses
mergedClosable (line ~192) but it is missing from the dependency array, causing
a stale-closure bug; update the useCallback dependency list to include
mergedClosable so the callback is recreated when closable state changes,
ensuring Escape key behavior reflects the latest mergedClosable value.
close ant-design/ant-design#55177
Summary by CodeRabbit