feat: element-plus style optimization #6379
Conversation
wyc001122
requested review from
anncwb,
vince292007,
mynetfan,
jinmao88 and
a team
as code owners
|
WalkthroughThis update expands the Element Plus demo page with a wide range of new UI component examples, introduces a new message box function, and enhances button and tag demonstrations. It also updates CSS variables for color tokens in design tokens and applies consistent border-radius styles to various Element UI components using a CSS variable. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DemoPage
participant ElementPlusComponents
participant MessageBox
User->>DemoPage: Interacts with UI (e.g., clicks button)
DemoPage->>ElementPlusComponents: Render UI components (Buttons, Tags, Tables, etc.)
User->>DemoPage: Triggers showMessageBox()
DemoPage->>MessageBox: Show alert dialog
MessageBox->>DemoPage: Callback on confirm
DemoPage->>ElementPlusComponents: Display info message
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/effects/hooks/src/use-design-tokens.ts (1)
208-214: 💡 Verification agent🧩 Analysis chain
Duplicate shade used for
light-7andlight-8
--el-color-*-light-7andlight-8now both read from the-200token.
That collapses two distinct tints into the same color and defeats Element Plus’s semantic scale.Double-check the intended mapping;
light-8is usually lighter thanlight-7(-100or-50in your scale).Also applies to: 228-234, 252-258, 272-278, 292-298
🏁 Script executed:
#!/bin/bash echo "=== light-7 mappings ===" rg -n --color=never "light-7" -C3 packages/effects/hooks/src/use-design-tokens.ts echo "" echo "=== light-8 mappings ===" rg -n --color=never "light-8" -C3 packages/effects/hooks/src/use-design-tokens.tsLength of output: 4497
Duplicate
-200mapping forlight-7andlight-8In packages/effects/hooks/src/use-design-tokens.ts, all five palettes (
danger,error,primary,success,warning) currently fall back to--*-200in light mode for bothlight-7andlight-8, collapsing two distinct tints into one:
- Danger (lines 205–210)
- Error (225–230)
- Primary (249–254)
- Success (269–274)
- Warning (289–294)
Typically,
light-8should be one step lighter thanlight-7(e.g.--*-100). Please update eachlight-8else-branch accordingly. For example:- '--el-color-danger-light-8': isDark.value - ? getCssVariableValue('--destructive-900') - : getCssVariableValue('--destructive-200'), + '--el-color-danger-light-8': isDark.value + ? getCssVariableValue('--destructive-900') + : getCssVariableValue('--destructive-100'),Repeat for
error,primary,success, andwarningpalettes.
🧹 Nitpick comments (2)
packages/styles/src/ele/index.css (2)
10-14: Fix typo in comment
/* propper 圆角 */⇒/* popper 圆角 */.
Minor, but keeping comments accurate avoids confusion.
45-84: Deep selector chain may break on Element-Plus updatesThe multi-line
.el-descriptions .el-descriptions__table.is-bordered …rules rely on internal DOM/class structure.
Element Plus has changed these internals before; a minor version bump could silently drop the rounding.Consider targeting the public CSS vars (
--el-border-radius-base) or wrapping the component in a utility class instead of overriding each cell.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web-ele/src/views/demos/element/index.vue(4 hunks)packages/effects/hooks/src/use-design-tokens.ts(5 hunks)packages/styles/src/ele/index.css(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Check (windows-latest)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web-ele/src/views/demos/element/index.vue (1)
80-86: Unintended array coercion and missing typing inoptionsgeneration
[idx % 10]creates a 1-element array which is coerced to string ("4"). A simple string template is clearer and avoids needless array creation. Additionally, specifying the item type helps TypeScript catch field name typos later.-const options = Array.from({ length: 1000 }).map((_, idx) => ({ - value: `Option ${idx + 1}`, - label: `${[idx % 10]}${idx}`, -})); +interface SelectOption { + value: string; + label: string; +} + +const options: SelectOption[] = Array.from({ length: 1000 }, (_, idx) => ({ + value: `Option ${idx + 1}`, + label: `${idx % 10}${idx}`, +}));Minor, but it shaves a tiny bit of memory and improves readability & type-safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web-ele/src/views/demos/element/index.vue(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
🔇 Additional comments (2)
apps/web-ele/src/views/demos/element/index.vue (2)
381-388:ElTreeSelectexpects tree-node objects – flatoptionswill not render
ElTreeSelect’sdataprop requires nodes with{ label, value, children? }.
Passing the flatoptionsarray will produce an empty tree.Consider mapping
optionsinto a minimal tree or useElSelectV2when a flat list suffices.const treeData = options.map((o) => ({ label: o.label, value: o.value, }));…and bind
:data="treeData".
362-367: 🟢 Good choice usingElSelectV2for 1 000 options
ElSelectV2provides virtual scrolling, so the large list won’t hurt performance.
| function messageBox() { | ||
| ElMessageBox.alert('This is a message', 'Title', { | ||
| confirmButtonText: 'OK', | ||
| callback: (action: any) => { | ||
| ElMessage({ | ||
| type: 'info', | ||
| message: `action: ${action}`, | ||
| }); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer promise-based handling for ElMessageBox instead of the deprecated callback API
ElMessageBox.alert already returns a Promise that resolves with the button action ('confirm' | 'cancel' | 'close'). Mixing the callback style with the Promise return value can lead to double-handling or unhandled-rejection noise when the user presses Esc / clicks the close icon.
-function messageBox() {
- ElMessageBox.alert('This is a message', 'Title', {
- confirmButtonText: 'OK',
- callback: (action: any) => {
- ElMessage({
- type: 'info',
- message: `action: ${action}`,
- });
- },
- });
-}
+import type { MessageBoxAction } from 'element-plus';
+
+function messageBox() {
+ ElMessageBox.alert('This is a message', 'Title', {
+ confirmButtonText: 'OK',
+ }).then((action: MessageBoxAction) => {
+ ElMessage({
+ type: 'info',
+ message: `action: ${action}`,
+ });
+ });
+}Benefits: cleaner flow, typed action, consistent with Element-Plus docs going forward.
📝 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.
| function messageBox() { | |
| ElMessageBox.alert('This is a message', 'Title', { | |
| confirmButtonText: 'OK', | |
| callback: (action: any) => { | |
| ElMessage({ | |
| type: 'info', | |
| message: `action: ${action}`, | |
| }); | |
| }, | |
| }); | |
| } | |
| import type { MessageBoxAction } from 'element-plus'; | |
| function messageBox() { | |
| ElMessageBox.alert('This is a message', 'Title', { | |
| confirmButtonText: 'OK', | |
| }).then((action: MessageBoxAction) => { | |
| ElMessage({ | |
| type: 'info', | |
| message: `action: ${action}`, | |
| }); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In apps/web-ele/src/views/demos/element/index.vue around lines 57 to 67, the
messageBox function uses the deprecated callback API for ElMessageBox.alert.
Refactor the function to use the Promise-based API by removing the callback
option and instead chaining a .then() handler on the returned Promise to handle
the action. This will provide a cleaner flow, properly typed action, and avoid
potential double-handling or unhandled rejections.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web-ele/src/views/demos/element/index.vue (1)
57-67:ElMessageBox.alertstill uses the deprecated callback API – please switch to the Promise interface
This exact point was raised in a previous review and remains unresolved. Switching removes double-handling risk and gives you a typedaction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web-ele/src/views/demos/element/index.vue(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
🔇 Additional comments (1)
apps/web-ele/src/views/demos/element/index.vue (1)
383-389: VerifyElTreeSelectdata shape
ElTreeSelectexpects hierarchical nodes withlabel,value, and optionalchildren.
The flatoptionsarray you pass lackschildren, so expanding nodes will show nothing.
If only a flat list is intended, consider usingElSelectinstead or transform the data accordingly.
| const type1 = ref(); | ||
| const date = ref(); | ||
| const options = Array.from({ length: 1000 }).map((_, idx) => ({ | ||
| value: `Option ${idx + 1}`, | ||
| label: `${[idx % 10]}${idx}`, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit types & fix redundant template‐string construction
ref()without a generic defaults toRef<undefined>, which later forcesanycasting when bound to Element-Plus models.- The label template literal currently does
${[idx % 10]}– the array wrapper is unnecessary and yields the same string via the array’stoString().
-const type1 = ref();
-const date = ref();
-const options = Array.from({ length: 1000 }).map((_, idx) => ({
- value: `Option ${idx + 1}`,
- label: `${[idx % 10]}${idx}`,
-}));
+const type1 = ref<string | null>(null);
+const date = ref<Date | null>(null);
+const options = Array.from({ length: 1000 }).map((_, idx) => ({
+ value: `Option ${idx + 1}`,
+ label: `${idx % 10}${idx}`,
+}));Benefits: keeps strict-type safety and avoids the small string-construction inefficiency.
📝 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.
| const type1 = ref(); | |
| const date = ref(); | |
| const options = Array.from({ length: 1000 }).map((_, idx) => ({ | |
| value: `Option ${idx + 1}`, | |
| label: `${[idx % 10]}${idx}`, | |
| })); | |
| const type1 = ref<string | null>(null); | |
| const date = ref<Date | null>(null); | |
| const options = Array.from({ length: 1000 }).map((_, idx) => ({ | |
| value: `Option ${idx + 1}`, | |
| label: `${idx % 10}${idx}`, | |
| })); |
🤖 Prompt for AI Agents
In apps/web-ele/src/views/demos/element/index.vue around lines 80 to 85, the
refs type1 and date are declared without explicit types, causing them to default
to Ref<undefined> and leading to any casting issues later. Also, the label
template string uses an unnecessary array wrapper around idx % 10. Fix this by
specifying explicit generic types for the refs to match their intended use and
simplify the label template string by removing the array wrapper, directly
embedding idx % 10.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web-ele/src/views/demos/element/index.vue (2)
125-136: Callback API forElMessageBox.alertwas already flagged – please migrate to the Promise-based variantPrevious review pointed out that the callback style is deprecated and can lead to double handling/unhandled rejections.
Refactor to:-import type { MessageBoxAction } from 'element-plus' -const showMessageBox = () => { - ElMessageBox.alert('This is a message', 'Title', { - confirmButtonText: 'OK', - callback: (action: any) => { - ElMessage({ type: 'info', message: `action: ${action}` }) - }, - }) -} +import type { MessageBoxAction } from 'element-plus' + +const showMessageBox = () => { + ElMessageBox.alert('This is a message', 'Title', { + confirmButtonText: 'OK', + }).then((action: MessageBoxAction) => { + ElMessage({ type: 'info', message: `action: ${action}` }) + }) +}Keeps the flow cleaner and fully typed.
78-85: 🛠️ Refactor suggestion
dateis typed asstringbut is bound to multiple date/time pickers
ElDatePicker,ElTimePicker, andElTimeSelectexpect Date / number / dayjs values, not plain strings.
Storing heterogeneous types in the same ref can introduce subtle runtime coercion issues.-const date = ref<string>(''); +const date = ref<Date | null>(null);You’ll need to adjust the initial model and clear behaviour accordingly.
🧹 Nitpick comments (1)
apps/web-ele/src/views/demos/element/index.vue (1)
88-93: Avoid recreating 1 000-item array on every computed re-evaluation
selectOptionshas no reactive dependencies, but each access still allocates a new array.
Extract it as a module-level constant or wrap inshallowRefto freeze the reference.-const selectOptions = computed(() => - Array.from({ length: 1000 }).map((_, idx) => ({ - value: `Option ${idx + 1}`, - label: `${idx % 10}${idx}`, - })), -); +const selectOptions = shallowRef( + Array.from({ length: 1000 }).map((_, idx) => ({ + value: `Option ${idx + 1}`, + label: `${idx % 10}${idx}`, + })), +);Reduces unnecessary allocations and keeps change detection cheap.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web-ele/src/views/demos/element/index.vue(1 hunks)packages/styles/src/ele/index.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/styles/src/ele/index.css
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
🔇 Additional comments (1)
apps/web-ele/src/views/demos/element/index.vue (1)
486-494: FlatselectOptionsfed intoElTreeSelect– verify expected structure
ElTreeSelectexpects a tree-shaped list withlabel,value, and optionalchildren.
Passing the 1-levelselectOptionsarray (label/value only) may silently break search or expansion features.Please confirm via manual test or convert the data:
const treeOptions = selectOptions.value.map(o => ({ ...o, children: [] }))If a flat list is intended, consider using
ElSelectV2instead.
Description
主要对Element Plus组件的主题色和圆角设置进行优化，实现与项目整体设计风格的统一。
并在
apps/web-ele/src/views/demos/element/index.vue添加了更多组件便于审查样式。颜色改动：
圆角统一
el-table改动前
el-table改动后
popper一系列组件改动前
popper一系列组件改动后
el-description改动前
el-description改动后
el-pagination改动前
el-pagination改动后
el-tabs改动前
el-tabs改动后
popconfirm 改动前
popconfirm 改动后
Type of change
Please delete options that are not relevant.
Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Style
Chores