feat: support keyborad event for Tour steps #86
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Walkthrough为 Tour 组件新增键盘交互：增加可选属性 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as 用户
participant KB as 键盘事件
participant Tour as Tour 组件
participant Host as 调用方/回调
participant Mask as Mask/Portal
Note over Tour,KB: 当 Tour.open 且 keyboard 为 true 时，注册 window keydown 处理器
User->>KB: 按键 (Esc / ArrowLeft / ArrowRight)
KB->>Tour: keydown -> keyboardHandler(e)
alt 事件目标为可编辑元素
Tour-->>KB: 忽略事件
else Esc 被按下
Tour->>Tour: handleEscClose -> handleClose(current)
Tour->>Host: 触发 onClose(current)
Tour->>Mask: 调用 Mask.onEsc(event)
else ArrowLeft / ArrowRight
Tour->>Tour: 计算 nextIndex
Tour->>Host: 调用 onInternalChange(nextIndex)
Host-->>Tour: （若受控）外部 onChange 同步状态
end
Note over Tour: 组件关闭或卸载时撤销 keydown 监听
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 @cactuser-Lu, 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! 此拉取请求通过引入键盘事件支持，显著提升了 Tour 组件的用户体验和可访问性。现在用户可以通过键盘快捷键关闭 Tour 或在不同步骤之间切换，这使得交互更加流畅和便捷。 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 to the Tour component, which is a great enhancement for accessibility. The implementation is well done. I have one suggestion to refactor the keyboard event handler to use modern browser APIs and improve its structure, which should also fix a minor bug. Additionally, there's a small typo in the pull request title ('keyborad' should be 'keyboard').
| const keyboardHandler = useEvent((e: KeyboardEvent) => { | ||
| if (keyboard && e.keyCode === KeyCode.ESC) { | ||
| if (mergedClosable !== null) { | ||
| e.stopPropagation(); | ||
| e.preventDefault(); | ||
| handleClose(); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (keyboard && e.keyCode === KeyCode.LEFT) { | ||
| if (mergedCurrent > 0) { | ||
| e.preventDefault(); | ||
| onInternalChange(mergedCurrent - 1); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (keyboard && e.keyCode === KeyCode.RIGHT) { | ||
| e.preventDefault(); | ||
| if (mergedCurrent < steps.length - 1) { | ||
| onInternalChange(mergedCurrent + 1); | ||
| } | ||
| return; | ||
| } | ||
| }); |
There was a problem hiding this comment.
The keyboard handler implementation can be improved for better readability, correctness, and to adhere to modern web standards.
- Use
e.keyinstead ofe.keyCode: ThekeyCodeproperty is deprecated. It's better to usee.keyfor future-proofing and consistency with modern web APIs. This change would also allow for the removal of theKeyCodeimport. - Conditional
preventDefault: Thee.preventDefault()for the right arrow key is called unconditionally. It should only be called when the step actually changes (i.e., inside theifblock), similar to the logic for the left arrow key. This prevents blocking the default browser behavior when at the last step. - Improved Structure: A
switchstatement one.keywould be more readable and maintainable than a series ofifstatements. Theif (keyboard)check can also be done once at the beginning of the function.
Here is a suggested refactoring that applies these points:
const keyboardHandler = useEvent((e: KeyboardEvent) => {
if (!keyboard) {
return;
}
switch (e.key) {
case 'Escape':
if (mergedClosable !== null) {
e.stopPropagation();
e.preventDefault();
handleClose();
}
break;
case 'ArrowLeft':
if (mergedCurrent > 0) {
e.preventDefault();
onInternalChange(mergedCurrent - 1);
}
break;
case 'ArrowRight':
if (mergedCurrent < steps.length - 1) {
e.preventDefault();
onInternalChange(mergedCurrent + 1);
}
break;
default:
// Do nothing
break;
}
});
![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
🧹 Nitpick comments (2)
src/interface.ts (1)
61-61: 建议添加 JSDoc 注释说明 keyboard 属性的用途。新增的
keyboard属性缺少文档注释，建议添加说明以提高 API 的可读性和可维护性。例如：open?: boolean; + /** Whether to enable keyboard interaction (Esc to close, Left/Right to navigate). Default: true */ keyboard?: boolean; defaultOpen?: boolean;src/Tour.tsx (1)
195-201: 考虑使用useEffect代替useLayoutEffect。事件监听器的注册和清理逻辑正确，依赖项数组也准确。但是，对于键盘事件监听器的注册，使用
useEffect而非useLayoutEffect更合适，因为这个操作不需要在浏览器绘制之前同步执行，使用useEffect可以避免阻塞视觉更新。建议修改：
- useLayoutEffect(() => { + useEffect(() => { if (!mergedOpen) return; window.addEventListener('keydown', keyboardHandler); return () => { window.removeEventListener('keydown', keyboardHandler); }; }, [mergedOpen, keyboardHandler]);注意：需要在文件顶部添加
useEffect的导入：- import { useMemo } from 'react'; + import { useMemo, useEffect } from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Tour.tsx(4 hunks)src/interface.ts(1 hunks)
🔇 Additional comments (3)
src/Tour.tsx (3)
7-8: 代码更改看起来不错！正确引入了
useEvent和KeyCode工具，用于实现键盘事件处理。
39-39: 代码更改看起来不错！
keyboard属性默认值为true是合理的设计，既启用了新功能，又保持了向后兼容性。
161-164: 代码更改看起来不错！将关闭逻辑提取为
handleClose函数是良好的代码组织实践，便于在多处复用（键盘处理、按钮点击、完成回调）。
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Tour.tsx (1)
168-204: 建议采用现代 API 并改进代码结构。当前实现已经解决了之前评审中关于
preventDefault调用时机和输入元素过滤的问题，这很好！但仍有改进空间：
- 使用
e.key替代e.keyCode：keyCode已被弃用，应使用e.key以符合现代 Web 标准。- 使用
switch语句：相比多个if语句，switch语句可读性和可维护性更好。应用以下差异来改进实现：
const keyboardHandler = useEvent((e: KeyboardEvent) => { // Ignore keyboard events from input-like elements to avoid interfering when typing const el = e.target as HTMLElement | null; if ( el?.tagName === 'INPUT' || el?.tagName === 'TEXTAREA' || el?.tagName === 'SELECT' || el?.isContentEditable ) { return; } - if (keyboard && e.keyCode === KeyCode.ESC) { - if (mergedClosable !== null) { - e.stopPropagation(); - e.preventDefault(); - handleClose(); - } - return; - } - - if (keyboard && e.keyCode === KeyCode.LEFT) { - if (mergedCurrent > 0) { - e.preventDefault(); - onInternalChange(mergedCurrent - 1); - } - return; - } - - if (keyboard && e.keyCode === KeyCode.RIGHT) { - if (mergedCurrent < steps.length - 1) { - e.preventDefault(); - onInternalChange(mergedCurrent + 1); - } - return; - } + if (!keyboard) { + return; + } + + switch (e.key) { + case 'Escape': + if (mergedClosable !== null) { + e.stopPropagation(); + e.preventDefault(); + handleClose(); + } + break; + case 'ArrowLeft': + if (mergedCurrent > 0) { + e.preventDefault(); + onInternalChange(mergedCurrent - 1); + } + break; + case 'ArrowRight': + if (mergedCurrent < steps.length - 1) { + e.preventDefault(); + onInternalChange(mergedCurrent + 1); + } + break; + default: + break; + } });
🧹 Nitpick comments (1)
src/Tour.tsx (1)
7-8: 建议使用现代的e.keyAPI 替代已弃用的KeyCode。
KeyCode已被弃用，建议使用e.key属性来检测按键。这样可以移除对KeyCode的导入依赖，并使代码更符合现代 Web 标准。应用以下差异以移除
KeyCode导入：import useLayoutEffect from '@rc-component/util/lib/hooks/useLayoutEffect'; import useEvent from '@rc-component/util/lib/hooks/useEvent'; -import KeyCode from '@rc-component/util/lib/KeyCode'; import useControlledState from '@rc-component/util/lib/hooks/useControlledState';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Tour.tsx(4 hunks)
🔇 Additional comments (4)
src/Tour.tsx (4)
39-39: 默认启用键盘支持是合理的设计！将
keyboard默认设置为true符合渐进增强原则，为大多数用户提供更好的可访问性体验。
93-93: 依赖项更新正确！将
setMergedCurrent添加到依赖数组中符合 React Hooks 的最佳实践，即使该函数引用通常是稳定的。
161-164: 提取handleClose函数提高了代码复用性！将关闭逻辑提取为独立函数是良好的重构实践，使得键盘处理和按钮点击可以共享相同的关闭逻辑。
206-212: 事件监听器的注册和清理实现正确！使用
useLayoutEffect在 tour 打开时注册全局键盘事件监听器，并在关闭或组件卸载时正确清理，避免了内存泄漏。
src/Tour.tsx
Outdated
| // Ignore keyboard events from input-like elements to avoid interfering when typing | ||
| const el = e.target as HTMLElement | null; | ||
| if ( | ||
| el?.tagName === 'INPUT' || |
There was a problem hiding this comment.
@aojunhao123 感觉这类检查还挺通用的，我们是不是可以抽到 rc-util 的 KeyCode util 里？
There was a problem hiding this comment.
可以的。经讨论，该工具函数顺带处理 IME 的 case
src/Tour.tsx
Outdated
| return; | ||
| } | ||
|
|
||
| if (keyboard && e.keyCode === KeyCode.ESC) { |
There was a problem hiding this comment.
直接用e.key === 'Escape' 来判断就行，keyCode已经是废弃api了，下面也同理
|
Someone is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
| if (KeyCode.isEditableTarget(e)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
这里我有个疑问，如果tour里有一些表单，比如input，此时我点击input聚焦后，再按下esc，是不是就直接return了？感觉这个是有点问题的，豆酱老师觉得呢 @zombieJ
There was a problem hiding this comment.
这不是预期的么……输入框里键盘操作都不应该控制 Tour 的切换
There was a problem hiding this comment.
这里我有个疑问，如果tour里有一些表单，比如input，此时我点击input聚焦后，再按下esc，是不是就直接return了？感觉这个是有点问题的
经讨论，在与表单交互时（非IME），esc依旧能够关闭tour，这个case会在Portal中通过加锁处理
src/Tour.tsx
Outdated
| return; | ||
| } | ||
|
|
||
| if (keyboard && e.key === 'Escape') { |
There was a problem hiding this comment.
使用 Portal 自带的 onEsc 如何？它会处理层叠逻辑
|
佬，还跟进吗 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #86 +/- ##
==========================================
+ Coverage 99.07% 99.16% +0.08%
==========================================
Files 10 10
Lines 217 240 +23
Branches 97 106 +9
==========================================
+ Hits 215 238 +23
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| } | ||
| return getPlacements(arrowPointAtCenter); | ||
| }, [builtinPlacements, arrowPointAtCenter]); | ||
| const handleClose = () => { |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/index.test.tsx (1)
1293-1325: 测试覆盖良好，可考虑补充箭头键导航测试。ESC 关闭功能的测试逻辑正确，正确验证了：
- 全局键盘事件监听（在 window 上模拟 keydown）
- onClose 回调被调用并传入当前步骤索引
- Tour 组件从 DOM 中移除
不过，根据 PR 目标，键盘支持还包括左右箭头键切换步骤。建议补充以下测试用例以提高覆盖率：
- 测试左右箭头键在多步骤 Tour 中的导航功能
- 测试
keyboard={false}时键盘事件是否被禁用- 测试组件卸载时键盘事件监听器是否正确清理
- 测试 Tour 关闭状态下按 ESC 不会触发 onClose
💡 可选的补充测试用例示例
it('should navigate steps with arrow keys', () => { const onChange = jest.fn(); const Demo = () => { const [current, setCurrent] = useState(0); return ( <Tour open current={current} onChange={(step) => { setCurrent(step); onChange(step); }} steps={[ { title: 'Step 1', description: 'First step' }, { title: 'Step 2', description: 'Second step' }, { title: 'Step 3', description: 'Third step' }, ]} /> ); }; render(<Demo />); // Press right arrow to go to next step fireEvent.keyDown(window, { key: 'ArrowRight' }); expect(onChange).toHaveBeenCalledWith(1); // Press left arrow to go to previous step fireEvent.keyDown(window, { key: 'ArrowLeft' }); expect(onChange).toHaveBeenCalledWith(0); }); it('should not respond to keyboard when keyboard prop is false', () => { const onClose = jest.fn(); render( <Tour open keyboard={false} onClose={onClose} steps={[{ title: 'Step 1', description: 'First step' }]} /> ); fireEvent.keyDown(window, { key: 'Escape' }); expect(onClose).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/index.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Socket Security: Pull Request Alerts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/index.test.tsx (2)
1293-1325: 建议补充测试用例：当 keyboard={false} 时按 ESC 键的行为。当前测试验证了按 ESC 键关闭 Tour 的基本功能，但缺少对
keyboard={false}场景的测试。根据 UX 最佳实践，即使禁用键盘导航，ESC 键通常仍应能关闭模态框/引导组件。建议补充测试用例明确这一行为预期。💡 建议的测试用例
+ it('should close tour when press ESC even if keyboard is disabled', () => { + const onClose = jest.fn(); + const Demo = () => { + const [open, setOpen] = useState(true); + return ( + <Tour + open={open} + keyboard={false} + onClose={(current) => { + setOpen(false); + onClose(current); + }} + steps={[ + { + title: '创建', + description: '创建一条数据', + }, + ]} + /> + ); + }; + + render(<Demo />); + expect(document.querySelector('.rc-tour')).toBeTruthy(); + + fireEvent.keyDown(window, { key: 'Escape' }); + + expect(onClose).toHaveBeenCalledWith(0); + expect(document.querySelector('.rc-tour')).toBeFalsy(); + });
1402-1404: 可选的代码风格优化：将 mockClear 放在独立行以提高可读性。当前
onChange.mockClear()的位置在逻辑上是正确的，但将其移到独立行能让测试的意图更清晰——表明这是在重置 mock 状态后开始新的断言。♻️ 建议的调整
// Press ArrowRight at last step - should not change - onChange.mockClear(); fireEvent.keyDown(window, { key: 'ArrowRight' }); + + onChange.mockClear(); expect(onChange).not.toHaveBeenCalled(); expect(document.querySelector('.rc-tour-title').innerHTML).toBe('step 2');或者在断言前清除：
// Press ArrowRight at last step - should not change + onChange.mockClear(); fireEvent.keyDown(window, { key: 'ArrowRight' }); expect(onChange).not.toHaveBeenCalled(); expect(document.querySelector('.rc-tour-title').innerHTML).toBe('step 2');
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/index.test.tsx
🔇 Additional comments (1)
tests/index.test.tsx (1)
1446-1493: 测试覆盖全面：正确验证了可编辑元素中的键盘事件处理。该测试用例考虑周到，验证了在 input 和 textarea 中按下方向键时不会触发 Tour 导航，这是良好的 UX 实践。测试正确地将事件触发在具体元素上（而非 window），准确模拟了真实的用户交互场景。
4 participants
🤔 This is a ...
支持键盘 Esc 关闭 \左右切换
Summary by CodeRabbit
新功能
修复
测试
✏️ Tip: You can customize this high-level summary in your review settings.