fix: select option style, add demo and test#1211
fix: select option style, add demo and test#1211Rain120 wants to merge 1 commit intoreact-component:masterfrom
Conversation
Summary of ChangesHello @Rain120, 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 improves the 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
|
概览添加了 Select 组件的内容选项样式设置文档和示例，重构了 SingleContent 组件使其始终将选中值包装在样式化的 div 中，并引入了全面的测试覆盖以验证样式、类名和标题属性的应用。 变更详情
预估代码审查工作量🎯 3 (中等) | ⏱️ ~25 分钟 可能相关的 PR
建议审查人
诗歌
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
|
@Rain120 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
![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 effectively addresses an issue with select option styling by simplifying the rendering logic in SingleContent.tsx. The change ensures that styles and class names from options are consistently applied to the selected value. The addition of a comprehensive demo and an extensive test suite is excellent, covering numerous scenarios and edge cases, which greatly improves the feature's reliability. I have one minor suggestion to improve the demo component by following React's best practices for state updates.
| setDynamicOptions([ | ||
| ...dynamicOptions, | ||
| { | ||
| value: searchValue, | ||
| label: searchValue, | ||
| style: { color: '#faad14' }, | ||
| }, | ||
| ]); |
There was a problem hiding this comment.
For state updates that depend on the previous state, it's a best practice to use the functional update form of useState's setter function. This avoids potential issues with stale state, for example if handleSearch were to be called in quick succession.
| setDynamicOptions([ | |
| ...dynamicOptions, | |
| { | |
| value: searchValue, | |
| label: searchValue, | |
| style: { color: '#faad14' }, | |
| }, | |
| ]); | |
| setDynamicOptions((prevDynamicOptions) => [ | |
| ...prevDynamicOptions, | |
| { | |
| value: searchValue, | |
| label: searchValue, | |
| style: { color: '#faad14' }, | |
| }, | |
| ]); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/examples/select-content-option.tsx`:
- Around line 22-33: handleSearch has a stale closure risk because it spreads
the current dynamicOptions when calling setDynamicOptions; change
setDynamicOptions to use the functional updater form to avoid reading an
out‑of‑date dynamicOptions (i.e., call setDynamicOptions(prev => { check prev
for existing value and return [...prev, newOption] }) while keeping the same new
option shape and the existing early-exit check logic; update references inside
handleSearch to use the prev argument instead of dynamicOptions.
- Around line 132-181: The "Custom Input Element" demo block is accidentally
nested inside the "Root Title Override" container: the <h3>Custom Input
Element...</h3> and its Select (mode="combobox", getInputElement,
options={dynamicOptions}) are inside the same outer div that wraps the Root
Title Override Select; split them into two sibling container divs so each demo
has its own wrapper and remove the extra closing </div> that currently closes
the Root Title Override wrapper incorrectly; locate the two Select usages and
their surrounding <div> blocks to separate them into distinct sections.
In `@src/SelectInput/Content/SingleContent.tsx`:
- Around line 74-83: The inner value div (`.rc-select-content-value`) always
gets title={optionTitle} causing duplicate tooltips when the outer content div
(`.rc-select-content`) also sets title in the !hasOptionStyle branch; update the
JSX in SingleContent.tsx so the inner div only receives the title when
hasOptionStyle is true (i.e. make the title on the inner element conditional on
hasOptionStyle), keeping the outer content div's existing logic unchanged;
reference the hasOptionStyle flag, optionTitle variable, the inner element
rendering displayValue.label and the optionClassName/optionStyle props to locate
and adjust the title assignment.
- Around line 74-83: The current JSX spreads mergedSearchValue visibility before
optionStyle so optionStyle can override it; in SingleContent.tsx adjust the
spread order so optionStyle is applied first and then the conditional visibility
from mergedSearchValue is spread (or explicitly set visibility when
mergedSearchValue is true) on the div that uses prefixCls-content-value,
optionClassName, optionStyle, mergedSearchValue, optionTitle and
displayValue.label to ensure the search-hide logic always wins.
| const handleSearch = (searchValue: string) => { | ||
| if (searchValue && !dynamicOptions.find((opt) => opt.value === searchValue)) { | ||
| setDynamicOptions([ | ||
| ...dynamicOptions, | ||
| { | ||
| value: searchValue, | ||
| label: searchValue, | ||
| style: { color: '#faad14' }, | ||
| }, | ||
| ]); | ||
| } | ||
| }; |
There was a problem hiding this comment.
handleSearch 中存在闭包陈旧引用问题
setDynamicOptions 使用了展开当前 dynamicOptions 的方式，在快速连续输入时可能引用到过时的 state。建议使用函数式更新：
♻️ 建议使用函数式更新
const handleSearch = (searchValue: string) => {
- if (searchValue && !dynamicOptions.find((opt) => opt.value === searchValue)) {
- setDynamicOptions([
- ...dynamicOptions,
+ if (searchValue) {
+ setDynamicOptions((prev) => {
+ if (prev.find((opt) => opt.value === searchValue)) return prev;
+ return [
+ ...prev,
{
value: searchValue,
label: searchValue,
style: { color: '#faad14' },
},
- ]);
+ ];
+ });
}
};🤖 Prompt for AI Agents
In `@docs/examples/select-content-option.tsx` around lines 22 - 33, handleSearch
has a stale closure risk because it spreads the current dynamicOptions when
calling setDynamicOptions; change setDynamicOptions to use the functional
updater form to avoid reading an out‑of‑date dynamicOptions (i.e., call
setDynamicOptions(prev => { check prev for existing value and return [...prev,
newOption] }) while keeping the same new option shape and the existing
early-exit check logic; update references inside handleSearch to use the prev
argument instead of dynamicOptions.
| <div style={{ marginBottom: 24 }}> | ||
| <h3>Root Title Override</h3> | ||
| <Select | ||
| value="override" | ||
| title="This title overrides option title" | ||
| style={{ width: 200 }} | ||
| options={[ | ||
| { | ||
| value: 'override', | ||
| label: 'Hover to see title', | ||
| title: 'Option title (will be overridden)', | ||
| style: { color: 'teal' }, | ||
| }, | ||
| ]} | ||
| /> | ||
| <h3>Custom Input Element with getInputElement (combobox mode)</h3> | ||
| <div style={{ marginBottom: 24 }}> | ||
| <p> | ||
| Use <code>getInputElement</code> to customize the input element. This only works with{' '} | ||
| <code>mode="combobox"</code>. Type to add new options dynamically. | ||
| </p> | ||
| <Select | ||
| mode="combobox" | ||
| style={{ width: 300 }} | ||
| suffixIcon={null} | ||
| showSearch | ||
| onSearch={handleSearch} | ||
| classNames={{ | ||
| prefix: 'custom-prefix', | ||
| suffix: 'custom-suffix', | ||
| }} | ||
| styles={{ | ||
| prefix: { marginRight: 8 }, | ||
| suffix: { marginLeft: 8 }, | ||
| }} | ||
| getInputElement={() => ( | ||
| <input | ||
| style={{ | ||
| border: '2px solid #1890ff', | ||
| borderRadius: 4, | ||
| padding: '4px 8px', | ||
| outline: 'none', | ||
| }} | ||
| placeholder="Type to add new option" | ||
| /> | ||
| )} | ||
| options={dynamicOptions} | ||
| /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
"Custom Input Element" 部分嵌套在 "Root Title Override" 的 div 内
Line 147 的 <h3>Custom Input Element...</h3> 及其内容位于 Line 132 开始的 "Root Title Override" div 中（该 div 在 Line 181 关闭）。这两个是独立的 demo 部分，应该各自有自己的容器 div。
♻️ 建议拆分为独立的 section
/>
+ </div>
+
+ <div style={{ marginBottom: 24 }}>
<h3>Custom Input Element with getInputElement (combobox mode)</h3>同时移除 Line 181 多余的 </div> 闭合标签。
🤖 Prompt for AI Agents
In `@docs/examples/select-content-option.tsx` around lines 132 - 181, The "Custom
Input Element" demo block is accidentally nested inside the "Root Title
Override" container: the <h3>Custom Input Element...</h3> and its Select
(mode="combobox", getInputElement, options={dynamicOptions}) are inside the same
outer div that wraps the Root Title Override Select; split them into two sibling
container divs so each demo has its own wrapper and remove the extra closing
</div> that currently closes the Root Title Override wrapper incorrectly; locate
the two Select usages and their surrounding <div> blocks to separate them into
distinct sections.
| <div | ||
| className={clsx(`${prefixCls}-content-value`, optionClassName)} | ||
| style={{ | ||
| ...(mergedSearchValue ? { visibility: 'hidden' } : {}), | ||
| ...optionStyle, | ||
| }} | ||
| title={optionTitle} | ||
| > | ||
| {displayValue.label} | ||
| </div> |
There was a problem hiding this comment.
当 hasOptionStyle 为 false 时，title 同时出现在内层 div 和外层 content div 上
Line 80 始终将 optionTitle 设置在内层 .rc-select-content-value div 上，而 Line 99 在 !hasOptionStyle 时也将 optionTitle 设置在外层 .rc-select-content div 上。这会导致在没有 option style 的情况下，两个嵌套元素都有 title 属性，tooltip 可能出现重复。
考虑仅在 hasOptionStyle 为 true 时才在内层 div 设置 title，与外层 content div 的逻辑互斥。
Also applies to: 99-99
🤖 Prompt for AI Agents
In `@src/SelectInput/Content/SingleContent.tsx` around lines 74 - 83, The inner
value div (`.rc-select-content-value`) always gets title={optionTitle} causing
duplicate tooltips when the outer content div (`.rc-select-content`) also sets
title in the !hasOptionStyle branch; update the JSX in SingleContent.tsx so the
inner div only receives the title when hasOptionStyle is true (i.e. make the
title on the inner element conditional on hasOptionStyle), keeping the outer
content div's existing logic unchanged; reference the hasOptionStyle flag,
optionTitle variable, the inner element rendering displayValue.label and the
optionClassName/optionStyle props to locate and adjust the title assignment.
optionStyle 可能覆盖搜索时的 visibility: 'hidden'
当 mergedSearchValue 为真时，visibility: 'hidden' 先被展开，但随后 optionStyle 被展开在后面。如果 option 的 style 中包含 visibility 属性，会覆盖掉隐藏逻辑，导致搜索时选中值仍然可见。
建议将 optionStyle 放在 visibility 之前展开，确保搜索隐藏逻辑优先：
🐛 建议修复样式合并顺序
style={{
+ ...optionStyle,
...(mergedSearchValue ? { visibility: 'hidden' } : {}),
- ...optionStyle,
}}🤖 Prompt for AI Agents
In `@src/SelectInput/Content/SingleContent.tsx` around lines 74 - 83, The current
JSX spreads mergedSearchValue visibility before optionStyle so optionStyle can
override it; in SingleContent.tsx adjust the spread order so optionStyle is
applied first and then the conditional visibility from mergedSearchValue is
spread (or explicitly set visibility when mergedSearchValue is true) on the div
that uses prefixCls-content-value, optionClassName, optionStyle,
mergedSearchValue, optionTitle and displayValue.label to ensure the search-hide
logic always wins.
1 participant
What I do
Fix bug
The bug from the commit
Initial
ant-design/ant-design#56932
Fixed
add demo and test
Summary by CodeRabbit
发布说明
新功能
改进
测试