chore: pass rc-select semantic #651
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 refactors 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
|
WalkthroughTreeSelect组件中修改了类型签名和参数传递机制，将SemanticName从具体类型别名改为BaseSelectSemanticName，并将classNames和styles的逐字段映射改为直接传递完整对象给底层BaseSelect组件。新增语义化定制测试套件验证。 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
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 (2)
🧰 Additional context used🧬 Code graph analysis (1)tests/Select.semantic.spec.tsx (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). (2)
🔇 Additional comments (6)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #651 +/- ##
=======================================
Coverage 99.83% 99.83%
=======================================
Files 17 17
Lines 619 619
Branches 177 190 +13
=======================================
Hits 618 618
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
The pull request refactors the TreeSelect component to utilize the BaseSelectSemanticName type from @rc-component/select/lib/BaseSelect for semantic class names, simplifying the component's props and improving maintainability. Additionally, a new test file Select.semantic.spec.tsx is added to verify the correct application of semantic class names and styles in both single and multiple modes.
| LegacyDataNode, | ||
| } from './interface'; | ||
| import useSearchConfig from './hooks/useSearchConfig'; | ||
|
|
There was a problem hiding this comment.
This change updates the SemanticName type to use BaseSelectSemanticName from @rc-component/select. This ensures that the TreeSelect component's semantic names are consistent with the base select component, promoting code reuse and reducing the risk of inconsistencies. This is a good change that improves maintainability and reduces redundancy.
Consider adding a comment explaining why this change was made, and where BaseSelectSemanticName is defined.
| import React from 'react'; | ||
| import { render } from '@testing-library/react'; | ||
| import TreeSelect, { TreeNode } from '../src'; | ||
|
|
||
| describe('TreeSelect.semantic', () => { | ||
| const createSingleSelect = (props = {}) => ( | ||
| <TreeSelect {...props}> | ||
| <TreeNode value="0" title="Node 0" key="0"> | ||
| <TreeNode value="0-0" title="Node 0-0" key="0-0" /> | ||
| <TreeNode value="0-1" title="Node 0-1" key="0-1" /> | ||
| </TreeNode> | ||
| <TreeNode value="1" title="Node 1" key="1" /> | ||
| </TreeSelect> | ||
| ); | ||
|
|
||
| const createMultipleSelect = (props = {}) => ( | ||
| <TreeSelect multiple {...props}> | ||
| <TreeNode value="0" title="Node 0" key="0"> | ||
| <TreeNode value="0-0" title="Node 0-0" key="0-0" /> | ||
| <TreeNode value="0-1" title="Node 0-1" key="0-1" /> | ||
| </TreeNode> | ||
| <TreeNode value="1" title="Node 1" key="1" /> | ||
| </TreeSelect> | ||
| ); | ||
|
|
||
| it('should support semantic classNames and styles in single mode', () => { | ||
| const classNames = { | ||
| prefix: 'custom-prefix', | ||
| suffix: 'custom-suffix', | ||
| input: 'custom-input', | ||
| clear: 'custom-clear', | ||
| placeholder: 'custom-placeholder', | ||
| content: 'custom-content', | ||
| }; | ||
|
|
||
| const styles = { | ||
| prefix: { color: 'red' }, | ||
| suffix: { color: 'blue' }, | ||
| input: { backgroundColor: 'yellow' }, | ||
| clear: { fontSize: '14px' }, | ||
| placeholder: { fontStyle: 'italic' }, | ||
| content: { fontWeight: 'bold' }, | ||
| }; | ||
|
|
||
| const { container } = render( | ||
| createSingleSelect({ | ||
| value: '0', | ||
| prefix: <span>Prefix</span>, | ||
| suffix: <span>Suffix</span>, | ||
| allowClear: true, | ||
| placeholder: 'Please select', | ||
| classNames, | ||
| styles, | ||
| }), | ||
| ); | ||
|
|
||
| // Test prefix | ||
| const prefixElement = container.querySelector(`.${classNames.prefix}`); | ||
| expect(prefixElement).toBeTruthy(); | ||
| expect(prefixElement).toHaveStyle(styles.prefix); | ||
|
|
||
| // Test suffix | ||
| const suffixElement = container.querySelector(`.${classNames.suffix}`); | ||
| expect(suffixElement).toBeTruthy(); | ||
| expect(suffixElement).toHaveStyle(styles.suffix); | ||
|
|
||
| // Test content | ||
| const contentElement = container.querySelector(`.${classNames.content}`); | ||
| expect(contentElement).toBeTruthy(); | ||
| expect(contentElement).toHaveStyle(styles.content); | ||
|
|
||
| // Test clear | ||
| const clearElement = container.querySelector(`.${classNames.clear}`); | ||
| expect(clearElement).toBeTruthy(); | ||
| expect(clearElement).toHaveStyle(styles.clear); | ||
| }); | ||
|
|
||
| it('should support semantic classNames and styles in multiple mode', () => { | ||
| const classNames = { | ||
| prefix: 'custom-prefix', | ||
| suffix: 'custom-suffix', | ||
| content: 'custom-content', | ||
| clear: 'custom-clear', | ||
| item: 'custom-item', | ||
| itemContent: 'custom-item-content', | ||
| itemRemove: 'custom-item-remove', | ||
| }; | ||
|
|
||
| const styles = { | ||
| prefix: { color: 'red' }, | ||
| suffix: { color: 'blue' }, | ||
| content: { fontWeight: 'bold' }, | ||
| clear: { fontSize: '14px' }, | ||
| item: { border: '1px solid green' }, | ||
| itemContent: { padding: '4px' }, | ||
| itemRemove: { color: 'orange' }, | ||
| }; | ||
|
|
||
| const { container } = render( | ||
| createMultipleSelect({ | ||
| value: ['0', '1'], | ||
| prefix: <span>Prefix</span>, | ||
| suffix: <span>Suffix</span>, | ||
| allowClear: true, | ||
| classNames, | ||
| styles, | ||
| }), | ||
| ); | ||
|
|
||
| // Test prefix | ||
| const prefixElement = container.querySelector(`.${classNames.prefix}`); | ||
| expect(prefixElement).toBeTruthy(); | ||
| expect(prefixElement).toHaveStyle(styles.prefix); | ||
|
|
||
| // Test suffix | ||
| const suffixElement = container.querySelector(`.${classNames.suffix}`); | ||
| expect(suffixElement).toBeTruthy(); | ||
| expect(suffixElement).toHaveStyle(styles.suffix); | ||
|
|
||
| // Test content | ||
| const contentElement = container.querySelector(`.${classNames.content}`); | ||
| expect(contentElement).toBeTruthy(); | ||
| expect(contentElement).toHaveStyle(styles.content); | ||
|
|
||
| // Test clear | ||
| const clearElement = container.querySelector(`.${classNames.clear}`); | ||
| expect(clearElement).toBeTruthy(); | ||
| expect(clearElement).toHaveStyle(styles.clear); | ||
|
|
||
| // Test items (multiple mode specific) | ||
| const items = container.querySelectorAll(`.${classNames.item}`); | ||
| expect(items.length).toBeGreaterThan(0); | ||
| items.forEach(item => { | ||
| expect(item).toHaveStyle(styles.item); | ||
| }); | ||
|
|
||
| // Test item contents (multiple mode specific) | ||
| const itemContents = container.querySelectorAll(`.${classNames.itemContent}`); | ||
| expect(itemContents.length).toBeGreaterThan(0); | ||
| itemContents.forEach(content => { | ||
| expect(content).toHaveStyle(styles.itemContent); | ||
| }); | ||
|
|
||
| // Test item remove buttons (multiple mode specific) | ||
| const removeButtons = container.querySelectorAll(`.${classNames.itemRemove}`); | ||
| expect(removeButtons.length).toBeGreaterThan(0); | ||
| removeButtons.forEach(button => { | ||
| expect(button).toHaveStyle(styles.itemRemove); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The addition of Select.semantic.spec.tsx provides valuable tests for ensuring the correct application of semantic class names and styles. The tests cover both single and multiple select modes, which is comprehensive. However, consider adding more test cases to cover edge cases and different scenarios, such as when certain class names or styles are not provided.
1 participant
Summary by CodeRabbit
发布说明
重构
测试