-
Notifications
You must be signed in to change notification settings - Fork 47
feat(spx-gui): uITag adapted for the new UI specification #2585
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
Conversation
Summary of ChangesHello @Overu, 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 addresses the adaptation of 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
|
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.
Code Review
This pull request refactors the UITag component to align with new UI specifications. The old UITag is renamed to UIChips to maintain compatibility, and a new, more versatile UITag component is introduced. The changes are well-structured, and the new component is demonstrated in the UI design documentation page.
I've left a few comments for improvement:
- In
UIChips.vue, I've suggested renaming the CSS class for consistency. - In the new
UITag.vue, I've pointed out a potential bug with event handling on the close icon and suggested a fix.
Overall, this is a great improvement to the UI component library.
| display: flex; | ||
| width: fit-content; | ||
| align-items: center; | ||
| height: 32px; // TODO: support different sizes |
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.
Hardcoded dimensions: Consider using CSS variables for consistency with UITag:
height: var(--ui-chips-height, 32px);
padding: var(--ui-chips-padding, 0 16px);This enables flexible sizing without code changes and maintains consistency across the component system.
spx-gui/src/components/ui/index.ts
Outdated
| export { default as UILoading } from './loading/UILoading.vue' | ||
| export { default as UIDetailedLoading } from './loading/UIDetailedLoading.vue' | ||
| export * from './message' | ||
| export { default as UIChips } from './UIChips.vue' |
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.
Missing type exports: Export the component types for TypeScript usage:
export { default as UIChips, type ChipsType } from './UIChips.vue'
export { default as UITag, type TagVariant, type TagColor, type TagSize } from './UITag.vue'This allows consumers to import types for proper type hints when using these components.
| import { UITextInput, UIIcon, UITag, UIPagination, UIButton, UISearchableModal, UIDivider } from '@/components/ui' | ||
| import { UITextInput, UIIcon, UIChips, UIPagination, UIButton, UISearchableModal, UIDivider } from '@/components/ui' | ||
| import { listAsset, AssetType, type AssetData, Visibility } from '@/apis/asset' | ||
| import { debounce } from 'lodash' |
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.
Bundle size optimization: Import from specific lodash modules to reduce bundle size:
import debounce from 'lodash/debounce'
// or
import { debounce } from 'lodash-es'This reduces bundle size from ~70KB (full lodash) to ~2KB (debounce only).
Code Review SummaryComprehensive review completed across code quality, performance, documentation, and security. The implementation is solid with good Vue 3 practices. Key findings: Critical Issues:
Important Improvements:
Overall well-implemented with minor improvements needed for production readiness. |
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.
这个 UIChip 对应的是设计 UIKit 里的哪个?我好像没找到
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.
这个是旧的 UITag,起了一个感觉更符合它的作用的名字。目前没有在规范中体现,之前同步给清清了,后续可以补齐相关的规范。
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.
它定位上跟 UITag 的区别是?
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.
UITag 交互性低,更偏向于 静态展示内容,比如 beta 展示
UIChip 交互性高,偏向于 动作触发,比如 点击条件过滤
我看大多数组件库是没有严格区分二者的。UIChip 如果作为 UITag 的一个 variant,行为和理解上也不会有什么区别。这里更多的是把 UIChip 作为旧的 UITag 的别名,且它的确是重交互。
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.
UIChip交互性高,偏向于 动作触发,比如 点击条件过滤
现在其实很像一个残缺版的 button..总感觉这个东西不应该存在
先这样吧,不过建议把二者的定位差异也在代码中注释说明下,选用的时候有个参考依据
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.
好的
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.
done, 0980360
Fixed #2583