-
Notifications
You must be signed in to change notification settings - Fork 280
feat(tag): update icon #2713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(tag): update icon #2713
Conversation
Caution Review failedThe pull request is closed. Walkthrough此拉取请求对 Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## V3.0 #2713 +/- ##
=======================================
Coverage 83.95% 83.95%
=======================================
Files 221 221
Lines 17842 17842
Branches 2675 2675
=======================================
Hits 14980 14980
Misses 2857 2857
Partials 5 5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/packages/tag/demos/taro/demo2.tsx
(2 hunks)src/packages/tag/tag.taro.tsx
(2 hunks)
🧰 Additional context used
🪛 Biome
src/packages/tag/tag.taro.tsx
[error] 142-143: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/packages/tag/demos/taro/demo2.tsx (2)
2-2
: 导入语句看起来不错!
图标导入的添加符合项目规范。
44-44
: closeIcon 的实现很合适!
使用 Failure 图标组件替换文本字符,并设置合适的尺寸,这样的改动提升了用户界面的视觉体验。
src/packages/tag/tag.taro.tsx (1)
1-1
: 导入语句组织得当!
导入语句的组织遵循了最佳实践:
- 类型导入在最前
- React相关导入其次
- 第三方库导入最后
Also applies to: 6-6, 8-8, 9-9
src/packages/tag/tag.taro.tsx
Outdated
<> | ||
<Close | ||
size={8} | ||
onClick={(e) => { | ||
setVisible(false) | ||
onClose && onClose(e) | ||
}} | ||
/> | ||
<Text | ||
onClick={(e) => { | ||
setVisible(false) | ||
onClose && onClose(e) | ||
}} | ||
className={`${classPrefix}-custom-icon`} | ||
> | ||
X | ||
</Text> | ||
</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
关闭图标实现需要优化
当前实现存在以下问题:
- Close图标和Text组件的点击处理逻辑重复
- 同时显示Close图标和'X'文本是多余的
- 缺少可访问性属性
- 回调函数调用可以使用可选链优化
建议按照以下方式重构代码:
- <>
- <Close
- size={8}
- onClick={(e) => {
- setVisible(false)
- onClose && onClose(e)
- }}
- />
- <Text
- onClick={(e) => {
- setVisible(false)
- onClose && onClose(e)
- }}
- className={`${classPrefix}-custom-icon`}
- >
- X
- </Text>
- </>
+ <Close
+ size={8}
+ className={`${classPrefix}-custom-icon`}
+ onClick={(e) => {
+ setVisible(false)
+ onClose?.(e)
+ }}
+ aria-label="关闭标签"
+ role="button"
+ />
优化说明:
- 移除冗余的Text组件
- 使用可选链调用onClose
- 添加aria-label和role属性提升可访问性
- 保持className以维持样式一致性
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
<> | |
<Close | |
size={8} | |
onClick={(e) => { | |
setVisible(false) | |
onClose && onClose(e) | |
}} | |
/> | |
<Text | |
onClick={(e) => { | |
setVisible(false) | |
onClose && onClose(e) | |
}} | |
className={`${classPrefix}-custom-icon`} | |
> | |
X | |
</Text> | |
</> | |
<Close | |
size={8} | |
className={`${classPrefix}-custom-icon`} | |
onClick={(e) => { | |
setVisible(false) | |
onClose?.(e) | |
}} | |
aria-label="关闭标签" | |
role="button" | |
/> |
🧰 Tools
🪛 Biome
[error] 142-143: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤔 这个变动的性质是?
🔗 相关 Issue
💡 需求背景和解决方案
☑️ 请求合并前的自查清单
Summary by CodeRabbit
新功能
Failure
图标,提升了可关闭标签的视觉表现。Close
图标和文本的结构。改进
Tag
组件的导入语句,确保onClose
和onClick
的默认属性为无操作函数。