feat: support allowClear #729
Conversation
|
@hawwei913 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
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为 InputNumber 组件新增可清除值的功能，包含属性声明（allowClear、onClear）、渲染与清除逻辑、样式类以及示例与文档更新。 Changes
Estimated code review effort🎯 3 (中等) | ⏱️ ~20 分钟 需重点检查：
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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🧠 Learnings (3)📓 Common learnings📚 Learning: 2024-09-29T06:18:11.993ZApplied to files:
📚 Learning: 2024-10-08T21:56:37.546ZApplied to files:
🔇 Additional comments (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 @hawwei913, 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 enhances 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
|
![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 a new allowClear feature to the InputNumber component, which is a great addition. The implementation is solid, and the new feature is well-documented with demos. I have a few suggestions to improve code clarity and fix some minor inconsistencies in the documentation. Specifically, I've suggested a refactoring in InputNumber.tsx to simplify the logic and pointed out a potentially unused CSS class. I've also recommended updates to the API documentation for the allowClear and onClear props to make them more accurate.
docs/api.md
Outdated
| </tr> | ||
| <tr> | ||
| <td>allowClear</td> | ||
| <td>boolean | { clearIcon?: React.ReactNode; clearValue: T }</td> |
There was a problem hiding this comment.
The type for allowClear is documented as having a required clearValue property when it's an object. However, in the implementation (src/InputNumber.tsx) and the demo (docs/demo/allow-clear.tsx), clearValue is optional. Please update the documentation to reflect that clearValue is optional to avoid confusion for users of the component.
It should be clearValue?: T.
| <td>boolean | { clearIcon?: React.ReactNode; clearValue: T }</td> | |
| <td>boolean | { clearIcon?: React.ReactNode; clearValue?: T }</td> |
| </tr> | ||
| <tr> | ||
| <td>onClear</td> | ||
| <td>Function</td> |
There was a problem hiding this comment.
The type for onClear is documented as Function, which is a bit vague. It would be more helpful to provide the actual function signature, which is () => void.
| <td>Function</td> | |
| <td>() => void</td> |
src/InputNumber.tsx
Outdated
| const iconNode = | ||
| typeof allowClear === 'object' && allowClear.clearIcon ? allowClear.clearIcon : '✖'; | ||
|
|
||
| const onInternalClear = () => { | ||
| const value = getMiniDecimal(typeof allowClear === 'object' && allowClear.clearValue); | ||
|
|
||
| triggerValueUpdate(value, false); | ||
| }; | ||
|
|
||
| clearButton = ( | ||
| <button | ||
| type="button" | ||
| tabIndex={-1} | ||
| onClick={() => { | ||
| onInternalClear(); | ||
| onClear?.(); | ||
| }} | ||
| onMouseDown={(e) => e.preventDefault()} | ||
| className={clsx(`${prefixCls}-clear-icon`, { | ||
| [`${clearIconCls}-hidden`]: !needClear, | ||
| [`${clearIconCls}-has-suffix`]: !!suffix, | ||
| })} | ||
| > | ||
| {iconNode} | ||
| </button> | ||
| ); |
There was a problem hiding this comment.
The logic for handling allowClear can be simplified to improve readability and reduce code duplication. You can destructure clearIcon and clearValue from an options object, and consolidate the onClick handler.
Additionally, the CSS class ${clearIconCls}-has-suffix is being added, but there are no corresponding styles for it in assets/index.less. If this class is not used, it should be removed to avoid dead code. I've kept it in the suggestion below, but please consider removing it if it's not needed.
const clearOptions = typeof allowClear === 'object' ? allowClear : {};
const { clearIcon = '✖', clearValue } = clearOptions;
const onInternalClear = () => {
const value = getMiniDecimal(clearValue);
triggerValueUpdate(value, false);
onClear?.();
};
clearButton = (
<button
type="button"
tabIndex={-1}
onClick={onInternalClear}
onMouseDown={(e) => e.preventDefault()}
className={clsx(`${prefixCls}-clear-icon`, {
[`${clearIconCls}-hidden`]: !needClear,
[`${clearIconCls}-has-suffix`]: !!suffix,
})}
>
{clearIcon}
</button>
);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
assets/index.less (1)
108-117: 建议增强按钮的交互样式。清除按钮缺少以下交互反馈：
- 缺少
cursor: pointer使鼠标悬停时显示为可点击状态- 缺少悬停（
:hover）和聚焦（:focus）样式以提供视觉反馈- 建议添加焦点可见性样式以改善键盘导航的可访问性
应用此差异以改进按钮样式：
&-clear-icon { padding: 0; font-size: 12px; background: none; border: none; + cursor: pointer; + color: rgba(0, 0, 0, 0.45); + transition: color 0.3s; + + &:hover { + color: rgba(0, 0, 0, 0.85); + } + + &:focus-visible { + outline: 2px solid #1890ff; + outline-offset: 2px; + } &-hidden { display: none; } }docs/demo/allow-clear.tsx (1)
33-52: 建议添加 onClear 回调的演示。当前演示涵盖了
allowClear的各种配置（基础用法、自定义 clearValue、自定义 clearIcon），但缺少对onClear回调的演示。建议添加一个示例来展示清除操作时的回调用法。可以在现有的某个 InputNumber 中添加 onClear 回调：
<InputNumber style={{ width: 200 }} value={value} onChange={onChange} allowClear={true} onClear={() => console.log('onClear triggered')} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
assets/index.less(1 hunks)docs/api.md(1 hunks)docs/demo/allow-clear.tsx(1 hunks)docs/example.md(1 hunks)src/InputNumber.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-09-29T06:18:11.993Z
Learnt from: bombillazo
Repo: react-component/input-number PR: 644
File: src/InputNumber.tsx:173-174
Timestamp: 2024-09-29T06:18:11.993Z
Learning: In `src/InputNumber.tsx`, within the `InternalInputNumber` component, the state variables `prevValueRef` and `inputValueRef` are typed as `string | number` to maintain consistency with existing code.
Applied to files:
docs/demo/allow-clear.tsxdocs/api.mdsrc/InputNumber.tsx
📚 Learning: 2024-10-08T21:56:37.546Z
Learnt from: bombillazo
Repo: react-component/input-number PR: 644
File: src/InputNumber.tsx:393-395
Timestamp: 2024-10-08T21:56:37.546Z
Learning: The `InputNumber` component does not use error states or messages; it is designed to prevent the value from updating if validation fails without displaying additional feedback to the user.
Applied to files:
docs/demo/allow-clear.tsxdocs/api.md
🔇 Additional comments (6)
docs/example.md (1)
63-66: 文档更新符合规范。新增的 allow-clear 示例章节格式与其他示例保持一致，引用路径正确。
docs/demo/allow-clear.tsx (1)
6-12: 状态管理正确。使用
string | number类型与 InputNumber 的值类型保持一致，onChange 处理逻辑清晰。src/InputNumber.tsx (4)
99-99: 类型定义准确。
allowClear和onClear的类型定义合理，支持布尔值或自定义配置对象，与 API 文档一致（需修正文档中的小错误）。Also applies to: 123-123
748-748: 清除按钮的渲染位置合理。清除按钮放置在输入框之后、suffix 之前，符合常见的 UI 模式和用户预期。
679-682: Verify whetheronClearshould fire only when the value actually changes.Currently, the
onClearcallback is invoked unconditionally whenever the clear button is clicked, even if the value does not change (for example, whenclearValueis the same as the current value). Clarify whether this is the intended behavior or ifonClearshould only be triggered when the value is actually modified.Consider checking whether
triggerValueUpdateindicates a value change occurred, and conditionally invokeonClearonly in that case.
669-673: No action needed — the code correctly handles clearing.When
allowClearistrueor an object without aclearValueproperty,getMiniDecimal(undefined)returns an emptyDecimalobject withisEmpty()returningtrue. The component correctly treats this as a cleared state, and downstream logic checksisEmpty()before converting to a numeric value. The implementation is correct.
| className={clsx(`${prefixCls}-clear-icon`, { | ||
| [`${clearIconCls}-hidden`]: !needClear, | ||
| [`${clearIconCls}-has-suffix`]: !!suffix, | ||
| })} |
There was a problem hiding this comment.
CSS 类名可能未定义。
第 686 行引用了 CSS 类 ${clearIconCls}-has-suffix，但在 assets/index.less 中未找到该类的定义。这可能导致该类名不起作用。
如果该类名用于在存在 suffix 时调整清除按钮的位置，请在 assets/index.less 中添加相应的样式定义。如果不需要，请移除此类名。
🤖 Prompt for AI Agents
In src/InputNumber.tsx around lines 684 to 687, the class
`${clearIconCls}-has-suffix` is being applied but no corresponding CSS exists in
assets/index.less; either add a style block for `.${clearIconCls}-has-suffix` in
assets/index.less that adjusts the clear-button positioning when a suffix is
present (mirror the existing `.${clearIconCls}-hidden`/base clear-icon rules and
include margin/positioning tweaks), or remove the `${clearIconCls}-has-suffix`
entry from the clsx call if no special styling is required.
1 participant
This pull request introduces a new "allow clear" feature to the
InputNumbercomponent, enabling users to clear the input value using a clear icon. The feature is highly customizable, allowing for a custom icon and a custom value to reset to when cleared. Documentation and demos have been updated to reflect this new functionality.New "allow clear" feature:
allowClearprop toInputNumber, supporting both boolean and object forms for custom clear icon and value, and theonClearcallback prop for handling clear actions. [1] [2]InputNumber, including rendering, event handling, and conditional display based on input state. [1] [2]assets/index.less.Documentation and demos:
docs/api.mdto describe the newallowClearandonClearprops.docs/demo/allow-clear.tsxand referenced it in the example documentation. [1] [2]Related Issue: ant-design/ant-design#50885
Summary by CodeRabbit
新功能
文档
样式
✏️ Tip: You can customize this high-level summary in your review settings.