Merged
Conversation
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并给出了一些整体性的反馈:
basePath现在在OptionSelectDropdownProps中是必填的;如果还有现有的OptionSelectDropdown/OptionSelectComboBox调用方没有在这个 PR 中一起更新,建议将basePath改为可选,或者提供一个合理的默认值,以避免意外的破坏性变更。- 即使
icon为 undefined,你现在也总是渲染AsyncIcon;建议只在存在图标时再进行渲染,以避免不必要的组件开销以及潜在的布局/ARIA 边缘问题。 - 现在
truncate类被应用在一个同时包含图标和标签的 flex 容器上;请确认文本仍然能按预期被截断,如果文本溢出的行为不再正确,考虑进行调整(例如,将 label 包在一个单独的span中,并在其上使用truncate和min-w-0)。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- `basePath` is now required on `OptionSelectDropdownProps`; if there are any existing callers of `OptionSelectDropdown`/`OptionSelectComboBox` that weren’t updated in this PR, consider making `basePath` optional or providing a sensible default to avoid unintended breakage.
- You always render `AsyncIcon` even when `icon` is undefined; consider conditionally rendering it only when an icon is present to avoid unnecessary component work and potential layout/ARIA edge cases.
- The `truncate` class is now applied to a flex container that includes both the icon and label; verify that this still truncates the text as expected and adjust (e.g., wrapping the label in a separate `span` with `truncate` and `min-w-0`) if text overflow no longer behaves correctly.
## Individual Comments
### Comment 1
<location> `src/components/OptionEditor.tsx:587-588` </location>
<code_context>
aria-controls={listboxId}
>
- <span className="truncate">{selectedOption?.label}</span>
+ <span className="flex items-center gap-1.5 truncate">
+ <AsyncIcon icon={selectedOption?.icon} basePath={basePath} className="w-4 h-4 object-contain flex-shrink-0" />
+ {selectedOption?.label}
+ </span>
</code_context>
<issue_to_address>
**suggestion:** 避免在 `icon` 为 undefined 时渲染 AsyncIcon,以减少不必要的计算和布局伪影。
由于 `selectedOption?.icon` 可能为 undefined,建议只在存在图标时才渲染 `AsyncIcon`,例如 `{selectedOption?.icon && <AsyncIcon ... />}`。这样可以在没有内容需要展示时避免额外开销,并防止空的图标占位影响布局。
Suggested implementation:
```typescript
<span className="flex items-center gap-1.5 truncate">
{selectedOption?.icon && (
<AsyncIcon
icon={selectedOption.icon}
basePath={basePath}
className="w-4 h-4 object-contain flex-shrink-0"
/>
)}
{selectedOption?.label}
</span>
```
```typescript
<span className="flex items-center gap-1.5 truncate">
{opt.icon && (
<AsyncIcon
icon={opt.icon}
basePath={basePath}
className="w-4 h-4 object-contain flex-shrink-0"
/>
)}
{opt.label}
```
</issue_to_address>帮我变得更有用!请对每条评论点 👍 或 👎,我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
basePathis now required onOptionSelectDropdownProps; if there are any existing callers ofOptionSelectDropdown/OptionSelectComboBoxthat weren’t updated in this PR, consider makingbasePathoptional or providing a sensible default to avoid unintended breakage.- You always render
AsyncIconeven wheniconis undefined; consider conditionally rendering it only when an icon is present to avoid unnecessary component work and potential layout/ARIA edge cases. - The
truncateclass is now applied to a flex container that includes both the icon and label; verify that this still truncates the text as expected and adjust (e.g., wrapping the label in a separatespanwithtruncateandmin-w-0) if text overflow no longer behaves correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- `basePath` is now required on `OptionSelectDropdownProps`; if there are any existing callers of `OptionSelectDropdown`/`OptionSelectComboBox` that weren’t updated in this PR, consider making `basePath` optional or providing a sensible default to avoid unintended breakage.
- You always render `AsyncIcon` even when `icon` is undefined; consider conditionally rendering it only when an icon is present to avoid unnecessary component work and potential layout/ARIA edge cases.
- The `truncate` class is now applied to a flex container that includes both the icon and label; verify that this still truncates the text as expected and adjust (e.g., wrapping the label in a separate `span` with `truncate` and `min-w-0`) if text overflow no longer behaves correctly.
## Individual Comments
### Comment 1
<location> `src/components/OptionEditor.tsx:587-588` </location>
<code_context>
aria-controls={listboxId}
>
- <span className="truncate">{selectedOption?.label}</span>
+ <span className="flex items-center gap-1.5 truncate">
+ <AsyncIcon icon={selectedOption?.icon} basePath={basePath} className="w-4 h-4 object-contain flex-shrink-0" />
+ {selectedOption?.label}
+ </span>
</code_context>
<issue_to_address>
**suggestion:** Avoid rendering AsyncIcon when `icon` is undefined to prevent unnecessary work and layout artifacts.
Because `selectedOption?.icon` can be undefined, consider only rendering `AsyncIcon` when an icon is present, e.g. `{selectedOption?.icon && <AsyncIcon ... />}`. This avoids work when there’s nothing to show and prevents an empty icon slot from impacting layout.
Suggested implementation:
```typescript
<span className="flex items-center gap-1.5 truncate">
{selectedOption?.icon && (
<AsyncIcon
icon={selectedOption.icon}
basePath={basePath}
className="w-4 h-4 object-contain flex-shrink-0"
/>
)}
{selectedOption?.label}
</span>
```
```typescript
<span className="flex items-center gap-1.5 truncate">
{opt.icon && (
<AsyncIcon
icon={opt.icon}
basePath={basePath}
className="w-4 h-4 object-contain flex-shrink-0"
/>
)}
{opt.label}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds icon support to select options in the OptionEditor component, enhancing the visual presentation of select dropdowns and comboboxes. The feature retrieves icon paths from CaseItem definitions and displays them using the existing AsyncIcon component, which already handles optional icons gracefully.
Changes:
- Extended option interface to include optional icon property
- Added basePath parameter to OptionSelectDropdown and OptionSelectComboBox components
- Integrated AsyncIcon display in both trigger buttons and dropdown/combobox option lists
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#74
疯狂地vibe(
由 Sourcery 提供的概要
为选项选择组件添加图标支持,并向下传递基础资源路径,以便在选项标签旁渲染图标。
新功能:
增强:
basePath属性,以支持通过AsyncIcon加载图标,并将其显示在触发器和列表中的选项标签旁。Original summary in English
Summary by Sourcery
Add icon support to option selection components and propagate base asset path for rendering icons alongside option labels.
New Features:
Enhancements: