fix: prevent page scroll when switching images with keyboard #493
Conversation
|
Someone is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Walkthrough重构 Preview 组件的键盘导航：移除对 KeyCode 的依赖，改为基于 event.key 处理 'ArrowLeft' / 'ArrowRight'，在触发左右切换时调用 event.preventDefault() 并调用对应 onActive；同时升级依赖 Changes
Sequence Diagram(s)（跳过 — 变更限于单组件内部键盘事件处理，涉及方少于 3 个） Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 分钟 Possibly related PRs
Suggested reviewers
兔兔诗歌
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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! This pull request addresses a user experience issue within the image preview component by preventing default browser actions when specific keyboard keys are pressed. Previously, using keys like 'Escape' or arrow keys for navigation could inadvertently trigger page scrolling. The changes ensure that these keyboard inputs are exclusively handled by the image preview, providing a smoother and more predictable interaction for users. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 correctly fixes the issue of page scrolling when switching images using the keyboard by adding event.preventDefault(). The change is straightforward and effective. I've also added a suggestion to refactor the keyboard event handling logic using a switch statement, which could improve code readability and maintainability. Overall, great job on fixing this bug.
src/Preview/index.tsx
Outdated
| if (keyCode === KeyCode.ESC) { | ||
| event.preventDefault(); | ||
| onClose?.(); | ||
| } | ||
|
|
||
| if (showLeftOrRightSwitches) { | ||
| if (keyCode === KeyCode.LEFT) { | ||
| event.preventDefault(); | ||
| onActive(-1); | ||
| } else if (keyCode === KeyCode.RIGHT) { | ||
| event.preventDefault(); | ||
| onActive(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
While the current implementation is correct, you could refactor this block to use a switch statement. This would make the code more structured, readable, and easier to maintain, especially if more keyboard shortcuts are added in the future. It also helps to reduce some code duplication by grouping the LEFT and RIGHT arrow key handling.
switch (keyCode) {
case KeyCode.ESC:
event.preventDefault();
onClose?.();
break;
case KeyCode.LEFT:
case KeyCode.RIGHT:
if (showLeftOrRightSwitches) {
event.preventDefault();
onActive(keyCode === KeyCode.LEFT ? -1 : 1);
}
break;
default:
break;
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #493 +/- ##
==========================================
- Coverage 99.41% 99.02% -0.39%
==========================================
Files 17 17
Lines 509 515 +6
Branches 152 155 +3
==========================================
+ Hits 506 510 +4
- Misses 3 5 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where using keyboard shortcuts (ESC, LEFT, RIGHT arrows) in the image preview component would trigger unwanted page scrolling. The fix adds event.preventDefault() calls to the keyboard event handler to prevent default browser behavior while maintaining the intended preview functionality.
Key Changes
- Added
event.preventDefault()for ESC key to prevent default behavior when closing preview - Added
event.preventDefault()for LEFT and RIGHT arrow keys to prevent page scrolling when switching between images
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Preview/index.tsx
Outdated
| event.preventDefault(); | ||
| onClose?.(); | ||
| } | ||
|
|
||
| if (showLeftOrRightSwitches) { | ||
| if (keyCode === KeyCode.LEFT) { | ||
| event.preventDefault(); | ||
| onActive(-1); | ||
| } else if (keyCode === KeyCode.RIGHT) { | ||
| event.preventDefault(); | ||
| onActive(1); |
There was a problem hiding this comment.
The keyboard event handling behavior, including the preventDefault calls, lacks test coverage. Consider adding tests to verify that:
- ESC key closes the preview and prevents default browser behavior
- LEFT/RIGHT arrow keys switch between images when showLeftOrRightSwitches is true
- preventDefault is properly called to prevent page scrolling
This would help ensure the fix for issue #56290 is properly validated and prevent regressions.
|
最好是等我这个pr #494 后再做你这个问题的修复吧，因为涉及到了portal侧的改动 @cactuser-Lu |
![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 (1)
src/Preview/index.tsx (1)
336-342: 建议将preventDefault()调用移到onActive()之前。当前实现在调用
onActive()之后才调用event.preventDefault()。虽然这样可以正常工作（因为onActive只是更新状态），但更好的做法是先调用preventDefault()，这样可以：
- 更早地阻止默认滚动行为
- 使代码意图更清晰
- 符合事件处理的最佳实践
🔎 建议的调整
if (keyCode === KeyCode.LEFT) { + event.preventDefault(); onActive(-1); - event.preventDefault(); } else if (keyCode === KeyCode.RIGHT) { + event.preventDefault(); onActive(1); - event.preventDefault(); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.jsonsrc/Preview/index.tsx
✅ Files skipped from review due to trivial changes (1)
- package.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Preview/index.tsx (1)
335-335: 考虑在执行操作之前调用 preventDefault。将
event.preventDefault()移至onActive()之前可以更清晰地表达意图：先阻止默认行为，再执行自定义操作。虽然当前代码功能正常，但这样的顺序更符合最佳实践。🔎 建议的调整
if (showLeftOrRightSwitches) { if (event.key === 'ArrowLeft') { - onActive(-1); event.preventDefault(); + onActive(-1); } else if (event.key === 'ArrowRight') { - onActive(1); event.preventDefault(); + onActive(1); } }Also applies to: 338-338
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Preview/index.tsx
🔇 Additional comments (1)
src/Preview/index.tsx (1)
330-342: Portal 组件可能不支持onEsc属性。根据对 @rc-component/portal 官方文档的调查，该库的公开 API 仅包含
open、getContainer和autoLock属性，不包括onEsc。代码中使用的onEsc: PortalProps['onEsc']属性可能基于未文档化或不存在的 API，存在以下问题：
- @rc-component/portal 官方文档中无
onEsc属性支持- package.json 中指定的版本 "^2.1.2" 在 npm 注册表中不存在（最新版本为 v2.0.0）
- 当前实现可能依赖于不受支持或不稳定的 API
建议检查是否需要在组件内部自行处理 ESC 键事件，而不是依赖 Portal 组件提供此功能。
Likely an incorrect or invalid review comment.
|
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.
|
3 participants
fix: ant-design/ant-design#56290
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.