chore: by pass rc-select semantic structure #630
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更新依赖 Changes
Sequence Diagram(s)（本次变更主要为类型扩展、依赖升级与测试迁移，不涉及运行时控制流变更，故无序列图。） Estimated code review effort🎯 3 (中等) | ⏱️ ~25 分钟 Possibly related PRs
Suggested reviewers
诗歌
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (3)
🔇 Additional comments (2)
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 @zombieJ, 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 primarily focuses on enhancing 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
|
|
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.
|
![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 semantic class names and styles for the Cascader component, enhancing its customizability. It also updates the rc-select dependency and includes corresponding test updates. The review focuses on ensuring the correctness and completeness of the new semantic elements and the accuracy of the updated tests.
| optionRender={option => `${option.label} - test`} | ||
| />, | ||
| ); | ||
| const input = container.querySelector('.rc-cascader-selection-search-input'); |
There was a problem hiding this comment.
The query selector is using .rc-cascader-selection-search-input which is a class name that was removed. This test will fail. Please update the query selector to use the correct class name.
const input = container.querySelector('.rc-cascader-input');
| const input = container.querySelector('.rc-cascader-selection-search-input'); | |
| const input = container.querySelector('.rc-cascader-input'); |
|
|
||
| // Click | ||
| fireEvent.click(document.querySelector('.rc-cascader-menu-item-content') as HTMLElement); | ||
| expect(document.querySelector('.rc-cascader-dropdown-hidden')).toBeTruthy(); |
There was a problem hiding this comment.
The query selector is using .rc-cascader-dropdown-hidden which is a class name that was removed. This test will fail. Please update the query selector to use the correct class name.
Consider using the expectOpen utility function that was added to keep the tests consistent.
| <Cascader value={['not', 'exist']} allowClear onChange={onChange} />, | ||
| ); | ||
|
|
||
| fireEvent.mouseDown(container.querySelector('.rc-cascader-clear-icon') as HTMLElement); |
There was a problem hiding this comment.
The query selector is using .rc-cascader-clear-icon which is a class name that was removed. This test will fail. Please update the query selector to use the correct class name.
fireEvent.mouseDown(container.querySelector('.rc-cascader-clear') as HTMLElement);
| fireEvent.mouseDown(container.querySelector('.rc-cascader-clear-icon') as HTMLElement); | |
| fireEvent.mouseDown(container.querySelector('.rc-cascader-clear') as HTMLElement); |
| ); | ||
|
|
||
| // Open and select | ||
| fireEvent.mouseDown(container.querySelector('.rc-cascader-selector') as HTMLElement); |
There was a problem hiding this comment.
The query selector is using .rc-cascader-selector which is a class name that was removed. This test will fail. Please update the query selector to use the correct class name.
fireEvent.mouseDown(container.querySelector('.rc-cascader') as HTMLElement);
| fireEvent.mouseDown(container.querySelector('.rc-cascader-selector') as HTMLElement); | |
| fireEvent.mouseDown(container.querySelector('.rc-cascader') as HTMLElement); |
| <Cascader checkable value={[['not'], ['exist']]} allowClear onChange={onChange} />, | ||
| ); | ||
|
|
||
| wrapper.find('.rc-cascader-selection-item-remove-icon').first().simulate('click'); |
There was a problem hiding this comment.
The query selector is using .rc-cascader-selection-item-remove-icon which is a class name that was removed. This test will fail. Please update the query selector to use the correct class name.
wrapper.find('.rc-cascader-selection-item-remove').first().simulate('click');
| wrapper.find('.rc-cascader-selection-item-remove-icon').first().simulate('click'); | |
| wrapper.find('.rc-cascader-selection-item-remove').first().simulate('click'); |
| type SemanticName = | ||
| | 'input' | ||
| | 'prefix' | ||
| | 'suffix' | ||
| | 'placeholder' | ||
| | 'content' | ||
| | 'item' | ||
| | 'itemContent' | ||
| | 'itemRemove'; |
There was a problem hiding this comment.
The SemanticName type definition has been expanded to include new semantic names. Ensure that all these names are consistently used throughout the component and that they cover all customizable elements. Also, consider adding documentation or comments to explain the purpose of each semantic name.
Missing documentation can lead to confusion and inconsistent usage.
src/Cascader.tsx
Outdated
| styles={{ | ||
| prefix: styles?.prefix, | ||
| suffix: styles?.suffix, | ||
| input: styles?.input, | ||
| placeholder: styles?.placeholder, | ||
| content: styles?.content, | ||
| item: styles?.item, | ||
| itemContent: styles?.itemContent, | ||
| itemRemove: styles?.itemRemove, | ||
| }} |
There was a problem hiding this comment.
Similar to the classNames prop, the styles prop is being used to apply semantic styles. Ensure that all the new semantic styles (placeholder, content, item, itemContent, itemRemove) are actually applied to the corresponding elements in the component's JSX.
Unused styles can lead to dead code and confusion.
| import { fireEvent, render } from '@testing-library/react'; | ||
| import { act, fireEvent, render } from '@testing-library/react'; | ||
| import KeyCode from '@rc-component/util/lib/KeyCode'; | ||
| import { expectOpen, selectOption } from './util'; |
There was a problem hiding this comment.
Adding a new utility function expectOpen and selectOption from ./util.ts. Ensure that these utility functions are well-documented and tested to avoid regressions.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #630 +/- ##
==========================================
- Coverage 99.83% 99.52% -0.31%
==========================================
Files 23 24 +1
Lines 614 635 +21
Branches 186 183 -3
==========================================
+ Hits 613 632 +19
- Misses 1 3 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/util.ts (1)
3-12: 考虑减少定时器推进时间。
jest.advanceTimersByTime(100000)推进了 100 秒，这个值似乎过大。除非有特定原因（如某些动画或延迟需要这么长时间），建议评估是否可以使用更小的值（如 1000-5000ms）来提高测试执行效率。tests/index.spec.tsx (1)
99-654: 建议考虑完成测试迁移或添加说明。当前文件中混合使用 Enzyme 和 React Testing Library 两种测试方法。虽然部分迁移可能是有意为之（如 PR 标题所示，针对特定的 rc-select 语义结构问题），但这会降低代码的可维护性：
- 开发者需要同时理解两个测试库
- 新增测试时可能不清楚应使用哪种方法
- 代码审查时需要关注两种不同的模式
建议考虑以下选项之一：
- 完成迁移：将剩余的 Enzyme 测试迁移到 RTL，保持测试方法的一致性
- 添加文档说明：在文件顶部添加注释，说明为什么采用混合方法，以及何时使用哪种测试方法
如需帮助完成迁移，我可以协助生成迁移代码。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/__snapshots__/search.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
package.json(1 hunks)src/Cascader.tsx(2 hunks)tests/fieldNames.spec.tsx(3 hunks)tests/index.spec.tsx(10 hunks)tests/search.spec.tsx(6 hunks)tests/selector.spec.tsx(4 hunks)tests/semantic.spec.tsx(3 hunks)tests/setup.js(1 hunks)tests/util.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/util.ts (1)
tests/setup.js (1)
menu(17-17)
tests/selector.spec.tsx (1)
tests/util.ts (1)
expectOpen(3-12)
tests/search.spec.tsx (1)
tests/util.ts (1)
expectOpen(3-12)
tests/index.spec.tsx (2)
tests/demoOptions.ts (1)
addressOptions(38-99)tests/util.ts (2)
expectOpen(3-12)selectOption(14-34)
⏰ 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). (2)
- GitHub Check: test / react component workflow
- GitHub Check: Analyze (javascript)
🔇 Additional comments (25)
tests/setup.js (1)
31-55: LGTM！MessageChannel mock 实现正确，通过
_target属性实现了双向端口通信，并使用setTimeout模拟异步消息传递。这为测试环境提供了合适的 postMessage 通信模拟。tests/fieldNames.spec.tsx (2)
42-42: LGTM！更新打开 cascader 的触发元素选择器，从
.rc-cascader-selector改为.rc-cascader，与新的语义结构保持一致。
79-79: LGTM！更新断言选择器从
.rc-cascader-selection-item改为.rc-cascader-content-value，正确反映了新的 DOM 结构。Also applies to: 100-100
tests/util.ts (1)
14-34: LGTM！
selectOption函数实现良好：
- 提供了清晰的错误消息来帮助调试
- 支持多种事件类型（click、doubleClick、mouseEnter）
- 使用标准的 DOM 查询方法
tests/selector.spec.tsx (3)
27-27: LGTM！更新清除操作的选择器从
.rc-cascader-clear-icon改为.rc-cascader-clear，与新的语义结构一致。Also applies to: 53-53, 63-63
45-46: LGTM！正确更新为使用新的
.rc-cascader选择器和expectOpen工具函数来验证打开状态，这与测试套件的现代化改造相符。Also applies to: 50-50
74-74: LGTM！更新移除操作的选择器从
.rc-cascader-selection-item-remove-icon改为.rc-cascader-selection-item-remove，与新的语义结构保持一致。tests/semantic.spec.tsx (2)
12-13: LGTM！为新的语义键（placeholder、content）添加了完整的测试覆盖，包括样式和类名验证。代码包含了适当的空值检查（针对可选的 placeholder 元素）。
Also applies to: 23-24, 42-71
73-156: LGTM！为多选模式添加了全面的语义测试，覆盖了所有新增的语义键（item、itemContent、itemRemove）。测试结构清晰，验证了样式和类名的正确应用。
tests/search.spec.tsx (4)
49-55: LGTM！添加了假定时器的设置和清理，这对于测试异步行为和动画是必要的。使用
beforeEach和afterEach确保了每个测试的隔离性。
285-296: LGTM！将测试从 Enzyme 迁移到 React Testing Library，使用
fireEvent和container.querySelector进行 DOM 交互，并使用expectOpen工具函数验证下拉框状态。这与测试套件的现代化方向一致。
366-389: LGTM！成功将此测试用例迁移到 React Testing Library 模式：
- 使用
fireEvent.change代替 Enzyme 的simulate- 使用
querySelectorAll和textContent进行断言- 与其他已迁移的测试保持一致
391-412: LGTM！这些测试用例正确地使用了 React Testing Library 的模式：
- 使用
toHaveValuematcher 进行输入值断言- 通过
querySelector获取 DOM 元素- 与新的测试工具集成良好
Also applies to: 414-440
src/Cascader.tsx (2)
155-163: LGTM！正确扩展了
SemanticName类型以包含新的语义键：placeholder、content、item、itemContent、itemRemove。这些新增项为组件提供了更细粒度的样式定制能力。
466-470: LGTM！新的语义键被正确地传递给
BaseSelect组件，使得自定义样式和类名能够应用到相应的 DOM 元素上。实现与类型定义保持一致。Also applies to: 476-480
package.json (1)
46-46: 依赖版本验证完成，无需更改。根据验证结果：
- ✓
@rc-component/select版本1.2.0-alpha.3存在于 npm 上- ✓ 该包没有已知的安全漏洞
该依赖版本的选择是有效且安全的。
tests/index.spec.tsx (9)
8-10: 导入语句正确！新增的 React Testing Library 导入和测试工具函数导入正确，支持从 Enzyme 到 RTL 的迁移。
30-48: RTL 迁移实现正确！测试已成功从 Enzyme 迁移到 React Testing Library，使用完整的事件序列（mouseDown → mouseUp → click）来准确模拟点击交互，并通过
expectOpen工具函数验证面板状态。
50-97: 选项选择测试迁移正确！测试正确使用了
selectOption工具函数和原生 DOM 查询方法，替换了 Enzyme 的 wrapper 方法。活动状态验证使用classList.contains是合适的。
170-226: 悬停触发测试迁移正确！测试正确使用
selectOption的mouseEnter事件类型，并在act()中包装jest.runAllTimers()以确保在断言前刷新定时器触发的状态更新。这是处理异步行为的最佳实践。
228-270: 状态重置测试迁移正确！两个测试（清除活动选择和恢复默认值）已正确迁移到 RTL，一致使用
fireEvent.click、selectOption和expectOpen工具函数。
272-327: 受控组件测试迁移正确！测试正确验证了
changeOnSelect和受控组件行为，RTL 迁移实现准确。
483-499: 悬停触发与 changeOnSelect 组合测试迁移正确！测试正确验证了当
expandTrigger="hover"且changeOnSelect启用时，点击仍会触发onChange的行为。
541-553: 双击关闭弹窗测试迁移正确！测试正确使用
selectOption的doubleClick事件类型，验证在changeOnSelect模式下双击关闭弹窗的行为。
1162-1186: ARIA 和 data 属性增强了可访问性！添加
aria-label、aria-labelledby和data-type属性是提升组件可访问性的良好实践。类型注解的引号风格变更也与 JavaScript 约定保持一致。
Summary by CodeRabbit
发布说明
新功能
依赖更新
测试