feat: enhance allowClear prop to support disabled#160
feat: enhance allowClear prop to support disabled#160zombieJ merged 3 commits intoreact-component:masterfrom
Conversation
Summary of ChangesHello, 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! 此拉取请求旨在增强输入组件的 allowClear 属性，使其能够通过一个内嵌的 disabled 属性来控制清除图标的禁用状态。这一改进简化了在某些场景下（例如在 ConfigProvider 中统一配置清除图标但又需要局部禁用清除功能时）的逻辑处理，提高了组件的灵活性和易用性。 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. Changelog
Activity
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
|
总体说明PR 修改了输入组件中清除按钮的行为：在 Walkthrough将 clear 按钮的可见性条件从仅检查 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 分钟 Possibly related PRs
Suggested reviewers
诗歌
🚥 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/BaseInput.tsx`:
- Around line 77-81: The current needClear calculation uses a truthy check on
value which treats 0 and 0n as empty; change the condition in BaseInput (the
needClear boolean calculation that references disabled, readOnly, value, and
allowClear) to check presence explicitly (e.g., value != null or value !==
undefined && value !== null) so numeric zero values count as "has value", and
extract this presence test into a small helper (e.g., isValuePresent or
hasValue) and reuse that helper at the other usage around Line 118 so both the
clear-button rendering and the style/state logic use the identical presence
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ec2e8e8-86e6-4d54-bc12-a3a7b598a8b0
📒 Files selected for processing (2)
src/BaseInput.tsxsrc/interface.ts
| !disabled && | ||
| !readOnly && | ||
| value && | ||
| !(typeof allowClear === 'object' && allowClear.disabled); |
There was a problem hiding this comment.
外层的 if (allowClear) 已经确保不为 null 了，因此这里就没有再判断了
| suffix?: CSSProperties; | ||
| }; | ||
| allowClear?: boolean | { clearIcon?: ReactNode }; | ||
| allowClear?: boolean | { disabled?: boolean; clearIcon?: ReactNode }; |
There was a problem hiding this comment.
antd 里是直接 import 的这个类型复用了
There was a problem hiding this comment.
我意思是 antd 里是否有 config 里带有 enabled or disabled 的属性可以参考一下。免得未来这个名字出现多种情况
There was a problem hiding this comment.
搜索了一下，大部分使用的是 disabled，比如 useClosable：
interface ClosableCollection {
closable?: ClosableType;
closeIcon?: ReactNode;
disabled?: boolean;
}比如 WaveConfig：
export interface WaveConfig {
disabled?: boolean;
showEffect?: ShowWaveEffect;
}而 enabled 只有一处使用：
export interface MaskConfig {
enabled?: boolean;
blur?: boolean;
closable?: boolean;
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #160 +/- ##
==========================================
+ Coverage 97.53% 97.57% +0.03%
==========================================
Files 4 4
Lines 203 206 +3
Branches 75 80 +5
==========================================
+ Hits 198 201 +3
Misses 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/index.test.tsx (1)
281-290: 测试用例正确验证了新功能的核心行为。测试逻辑与
src/BaseInput.tsx中的needClear条件（第 81 行：!(typeof allowClear === 'object' && allowClear.disabled)）相匹配，并且遵循了现有测试（第 265-279 行）的模式。建议考虑增加补充测试用例以提高覆盖率：
disabled: false时应显示清除图标 - 验证显式设置为false时的正确行为disabled与clearIcon组合使用 - 验证 PR 描述中的主要用例💡 建议的补充测试用例
it('should not support allowClear when allowClear is disabled', () => { const { container } = render( <Input prefixCls="rc-input" allowClear={{ disabled: true }} defaultValue="111" />, ); expect(container.querySelector('.rc-input-clear-icon-hidden')).toBeTruthy(); }); + + it('should support allowClear when allowClear.disabled is false', () => { + const { container } = render( + <Input + prefixCls="rc-input" + allowClear={{ disabled: false }} + defaultValue="111" + />, + ); + expect(container.querySelector('.rc-input-clear-icon-hidden')).toBeFalsy(); + }); + + it('should hide clear icon but keep custom clearIcon when disabled', () => { + const { container } = render( + <Input + prefixCls="rc-input" + allowClear={{ disabled: true, clearIcon: <span className="custom-clear">X</span> }} + defaultValue="111" + />, + ); + expect(container.querySelector('.rc-input-clear-icon-hidden')).toBeTruthy(); + expect(container.querySelector('.custom-clear')).toBeTruthy(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/index.test.tsx` around lines 281 - 290, Add two supplementary tests in tests/index.test.tsx mirroring existing patterns: 1) render <Input allowClear={{ disabled: false }} defaultValue="111" /> and assert the clear icon is shown (opposite of the current test) to validate the needClear logic in BaseInput.tsx (condition using !(typeof allowClear === 'object' && allowClear.disabled)); 2) render <Input allowClear={{ disabled: true, clearIcon: <span>X</span> }} defaultValue="111" /> (and a variant with disabled: false) to verify clearIcon is respected when combined with disabled flag; use the same container.querySelector/assert pattern as surrounding tests to check presence/absence of .rc-input-clear-icon-hidden or the custom clear icon.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/index.test.tsx`:
- Around line 281-290: Add two supplementary tests in tests/index.test.tsx
mirroring existing patterns: 1) render <Input allowClear={{ disabled: false }}
defaultValue="111" /> and assert the clear icon is shown (opposite of the
current test) to validate the needClear logic in BaseInput.tsx (condition using
!(typeof allowClear === 'object' && allowClear.disabled)); 2) render <Input
allowClear={{ disabled: true, clearIcon: <span>X</span> }} defaultValue="111" />
(and a variant with disabled: false) to verify clearIcon is respected when
combined with disabled flag; use the same container.querySelector/assert pattern
as surrounding tests to check presence/absence of .rc-input-clear-icon-hidden or
the custom clear icon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b578598-8999-4c67-b588-8164bc278372
📒 Files selected for processing (1)
tests/index.test.tsx
2 participants
在一些情况下，allowClear 和 clearIcon 分别由不同的变量控制。
以前的写法（略显杂乱）：
现在的写法（优雅一些）：
尤其是在 ConfigProvider 里，我们更常需要替换 clearIcon 而不改变 allowClear 的开关状态。因此增加一个单独的 disabled 属性，有利于我们更灵活处理多种情况。
Summary by CodeRabbit
新功能
变更
测试