feat(select-wrapper, base-select, tree-select, grid-select):select-wrapper mobile-first development #3881
Conversation
WalkthroughThis PR introduces a new select-wrapper module infrastructure across design systems (Aurora and Saas), adds mobile-first component implementations for SelectWrapper with extensive configuration options, and extends existing components with new props (initLabel, inputBoxType, showEmptyValue) and updated label initialization logic. Multiple prop renames standardize naming conventions across components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/vue/src/select-wrapper/src/mobile-first.vue (2)
2-9: Consider adding a:keyon thev-fortemplate for slots
<template v-for="slotName in slotNames" ...>currently has no:key. Vue will still work, but adding a stable key avoids potential warnings and makes diffing clearer ifslotNameschanges.- <template v-for="slotName in slotNames" v-slot:[slotName]="slotProps"> + <template v-for="slotName in slotNames" v-slot:[slotName]="slotProps" :key="slotName"> <slot :name="slotName" v-bind="slotProps"></slot> </template>
306-314: Reduce ambiguity by renaming the importedsetuphelperUsing
setupboth as the imported helper and as the component’ssetupmethod works (the method name doesn’t create a local binding), but it’s easy for readers to think the innersetup(...)call is recursive.You could rename the import to clarify intent:
-import { props, $prefix, defineComponent, setup } from '@opentiny/vue-common' +import { props, $prefix, defineComponent, setup as useSetup } from '@opentiny/vue-common' // ... - setup(props, context) { + setup(props, context) { const components = { TinyBaseSelect, TinyTreeSelect, TinyGridSelect } - return setup({ props, context, renderless, api, extendOptions: { components } }) as any + return useSetup({ props, context, renderless, api, extendOptions: { components } }) as any }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/design/saas/src/select-wrapper/icon-loading.svgis excluded by!**/*.svg
📒 Files selected for processing (10)
packages/design/aurora/src/select-wrapper/index.ts(1 hunks)packages/design/saas/src/select-wrapper/index.ts(1 hunks)packages/renderless/src/base-select/index.ts(1 hunks)packages/renderless/src/select-wrapper/vue.ts(1 hunks)packages/vue/src/base-select/src/mobile-first.vue(2 hunks)packages/vue/src/grid-select/src/mobile-first.vue(1 hunks)packages/vue/src/input/src/mobile-first.vue(2 hunks)packages/vue/src/select-wrapper/src/index.ts(2 hunks)packages/vue/src/select-wrapper/src/mobile-first.vue(1 hunks)packages/vue/src/select-wrapper/src/pc.vue(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-11-25T03:43:05.285Z
Learnt from: Davont
Repo: opentiny/tiny-vue PR: 2513
File: packages/vue/src/huicharts/huicharts-histogram/src/chart-histogram.vue:33-36
Timestamp: 2024-11-25T03:43:05.285Z
Learning: 在 Tiny Vue 代码库中，使用 `chart-core` 中的 `huiChartOption` 的组件，不应在其 `data` 中定义 `huiChartOption` 或 `option`，而是应该依赖 `chart-core` 提供的 `huiChartOption`。
Applied to files:
packages/design/aurora/src/select-wrapper/index.tspackages/design/saas/src/select-wrapper/index.ts
📚 Learning: 2024-11-25T03:24:05.740Z
Learnt from: Davont
Repo: opentiny/tiny-vue PR: 2513
File: packages/vue/src/huicharts/huicharts-sunburst/src/chart-sunburst.vue:30-32
Timestamp: 2024-11-25T03:24:05.740Z
Learning: 在位于`packages/vue/src/huicharts/huicharts-sunburst/src/chart-sunburst.vue`的组件中，当使用`chart-core`时，应删除错误的`option`定义，使用`chart-core`中的`huiChartOption`。
Applied to files:
packages/design/aurora/src/select-wrapper/index.tspackages/design/saas/src/select-wrapper/index.ts
📚 Learning: 2024-11-25T03:43:19.322Z
Learnt from: Davont
Repo: opentiny/tiny-vue PR: 2513
File: packages/vue/src/huicharts/huicharts-heatmap/src/chart-heatmap.vue:38-40
Timestamp: 2024-11-25T03:43:19.322Z
Learning: 在 Vue.js 组件（如 `chart-heatmap.vue`）中，使用来自 `chart-core` 的 `huiChartOption` 来管理图表选项，不要在 `data()` 中声明 `option`。
Applied to files:
packages/design/aurora/src/select-wrapper/index.tspackages/design/saas/src/select-wrapper/index.ts
📚 Learning: 2024-11-25T03:24:33.808Z
Learnt from: Davont
Repo: opentiny/tiny-vue PR: 2513
File: packages/vue/src/huicharts/huicharts-sunburst/src/chart-sunburst.vue:30-32
Timestamp: 2024-11-25T03:24:33.808Z
Learning: 对于使用了来自`opentiny/vue-huicharts-core`中`Core` mixin的组件，`huiChartOption`数据属性已由`chart-core`提供，因此无需在组件的`data()`中声明。
Applied to files:
packages/design/saas/src/select-wrapper/index.ts
🧬 Code graph analysis (1)
packages/design/saas/src/select-wrapper/index.ts (1)
packages/renderless/src/select-wrapper/vue.ts (1)
api(1-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (9)
packages/renderless/src/select-wrapper/vue.ts (1)
17-33: Class deduplication anddataTagaddition look correctUsing
Array.from(new Set(classArray))here cleanly removes duplicate class entries while preserving order and keeps existing behavior for string/arrayclassbindings. AddingdataTag: 'tiny-select'after spreadingpropsandrestAttrsenforces a consistent identifier for downstream consumers, even if a conflictingdataTagwas provided upstream. No changes needed from my side.packages/renderless/src/base-select/index.ts (1)
1252-1255: LGTM! Fallback label initialization added.The additional branch correctly handles the case where
state.selectedis not present butmodelValueandinitLabelexist. This ensures that theselectedLabelis properly initialized frominitLabelin scenarios where selection state hasn't been fully established yet.packages/vue/src/input/src/mobile-first.vue (1)
97-105: LGTM! Conditional styling for inputBoxType implemented correctly.The conditional rendering logic properly applies different border styles based on the
inputBoxTypeprop:
'underline': removes side and top borders, keeping only bottom border- Other values: applies rounding based on the presence of prepend/append slots
packages/vue/src/base-select/src/mobile-first.vue (1)
763-766: LGTM! New props properly integrated.The three new props (
initLabel,inputBoxType,showEmptyValue) are correctly declared and passed through to theTinyInputcomponent (lines 305-306). These extend the component's API surface to support:
- External label initialization via
initLabel- Input box styling control via
inputBoxType- Empty value display handling via
showEmptyValuepackages/vue/src/select-wrapper/src/pc.vue (1)
266-270: LGTM! Prop naming convention improved.The prop has been correctly renamed from
InputBoxTypetoinputBoxType, following JavaScript/Vue camelCase naming conventions. The type, default value, and validator remain unchanged.packages/vue/src/grid-select/src/mobile-first.vue (1)
40-41: Verify thatbuildSelectConfig()andbuildRadioConfig()functions are properly implemented and exported from the renderless API.The grid configuration bindings have been changed from static state properties to dynamic builder function calls. Ensure these functions are defined in the renderless module (
@opentiny/vue-renderless/grid-select/vue) and properly accessible within the component context, as missing or incorrectly scoped implementations would cause runtime errors.packages/design/aurora/src/select-wrapper/index.ts (1)
48-98: Verify API method availability and compare Aurora/Saas implementations for duplication.The
toggleCheckAllfunction calls API methods (setSoftFocus,updateModelValue,directEmitChange) that need verification. Additionally, check if this implementation duplicates logic in the Saas theme's select-wrapper module. If duplication exists, extract the common logic into a shared utility function.packages/vue/src/select-wrapper/src/index.ts (1)
15-15: Verify mobile-first template configuration.The template import uses a virtual-template loader with
?pc|mobile-firstquery parameters. Confirm that the mobile-first variant is properly configured in the template loader configuration and that the corresponding template variant is available.packages/vue/src/select-wrapper/src/mobile-first.vue (1)
254-257: Verify thatstopPropagationis intentionally tri‑state (Boolean +undefined)Defining a Boolean prop with
default: undefinedgives you three states (true/false/ “not set”). That’s useful if the renderless logic distinguishes “unset” from an explicitfalse, but it also means any boolean operations must guard againstundefined.Please double‑check the renderless/select‑wrapper logic to confirm it:
- Explicitly handles
undefinedas a distinct case, and- Never assumes
stopPropagationis always a strict boolean without normalization (e.g., using!!props.stopPropagationwhere needed).
| import { iconChevronDown, iconPlus } from '@opentiny/vue-icon' | ||
| import loadingIcon from './icon-loading.svg' | ||
|
|
||
| export default { | ||
| // 虚拟滚动的默认options不一致 | ||
| baseOpts: { optionHeight: 34, limit: 20 }, | ||
| icons: { | ||
| dropdownIcon: iconChevronDown(), | ||
| addIcon: iconPlus(), | ||
| loadingIcon | ||
| }, | ||
| state: { | ||
| sizeMap: { | ||
| default: 28, | ||
| mini: 24, | ||
| small: 28, | ||
| medium: 32 | ||
| }, | ||
| spacingHeight: 4, | ||
| initialInputHeight: 28, | ||
| // 显示清除等图标时，不隐藏下拉箭头时 | ||
| autoHideDownIcon: false, | ||
| delayBlur: true | ||
| }, | ||
| props: { | ||
| tagType: 'info', | ||
| stopPropagation: true | ||
| }, | ||
| renderless: (props, hooks, { emit }, api) => { | ||
| const state = api.state | ||
|
|
||
| return { | ||
| computedCollapseTagSize: () => { | ||
| let size = 'small' | ||
|
|
||
| if (~['small', 'mini'].indexOf(state.selectSize)) { | ||
| size = state.selectSize | ||
| } else if (~['medium', 'default'].indexOf(state.selectSize)) { | ||
| size = 'small' | ||
| } | ||
|
|
||
| return size | ||
| }, | ||
| // aui 的勾选未处理disabled的选项，故此放这里。 | ||
| toggleCheckAll: (filtered) => { | ||
| const getEnabledValues = (options) => { | ||
| let values = [] | ||
|
|
||
| for (let i = 0; i < options.length; i++) { | ||
| if (!options[i].state.disabled && !options[i].state.groupDisabled && options[i].state.visible) { | ||
| values.push(options[i].value) | ||
| } | ||
| } | ||
|
|
||
| return values | ||
| } | ||
|
|
||
| let value | ||
| const enabledValues = getEnabledValues(state.options) | ||
|
|
||
| if (filtered) { | ||
| if (state.filteredSelectCls === 'check' || state.filteredSelectCls === 'halfselect') { | ||
| value = Array.from(new Set([...state.modelValue, ...enabledValues])) | ||
| } else { | ||
| value = state.modelValue.filter((val) => !enabledValues.includes(val)) | ||
| } | ||
| } else { | ||
| if (state.selectCls === 'check') { | ||
| value = enabledValues | ||
| } else if (state.selectCls === 'halfselect') { | ||
| const unchecked = state.options.filter((item) => !item.state.disabled && item.state.selectCls === 'check') | ||
|
|
||
| unchecked.length ? (value = enabledValues) : (value = []) | ||
| } else if (state.selectCls === 'checked-sur') { | ||
| value = [] | ||
| } | ||
| } | ||
|
|
||
| const requiredValue = [] | ||
| if (props.multiple) { | ||
| state.options.forEach((opt) => { | ||
| if (opt.required) requiredValue.push(opt.value) | ||
| }) | ||
| } | ||
|
|
||
| if (Array.isArray(value)) { | ||
| value = requiredValue.concat(value.filter((val) => !requiredValue.find((requireVal) => requireVal === val))) | ||
| } | ||
|
|
||
| api.setSoftFocus() | ||
|
|
||
| state.isSilentBlur = true | ||
| api.updateModelValue(value) | ||
| api.directEmitChange(value) | ||
| }, | ||
| // aurora 禁用和只展示的时候都是tagText，默认主题是 isDisplayOnly 才显示tagText | ||
| computedShowTagText: () => { | ||
| return state.isDisabled || state.isDisplayOnly | ||
| }, | ||
| // aurora 禁用已选项无效果，必选不显示关闭图标 | ||
| isTagClosable: (item) => { | ||
| return !item.required | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication detected between Aurora and Saas implementations.
This file is nearly identical to packages/design/aurora/src/select-wrapper/index.ts, with only minor differences in state.sizeMap values and additional icons. The toggleCheckAll function (lines 45-95) is duplicated verbatim.
Consider refactoring to reduce duplication:
- Extract common logic into a shared base module
- Use theme-specific overrides for differences (sizeMap, icons)
- Move the
toggleCheckAllimplementation to the shared renderless layer
Example structure:
// packages/design/common/select-wrapper-base.ts
export const createSelectWrapperConfig = (themeConfig) => ({
baseOpts: themeConfig.baseOpts,
icons: themeConfig.icons,
state: themeConfig.state,
props: themeConfig.props,
renderless: createRenderless(themeConfig)
})Then each theme file would only define its specific configuration values.
| inputBoxType: { | ||
| type: String, | ||
| default: 'input', | ||
| default: 'normal', | ||
| validator: (value: string) => ['input', 'underline'].includes(value) | ||
| }, |
There was a problem hiding this comment.
Critical: Default value fails prop validation.
The inputBoxType prop has a default value of 'normal', but the validator only allows ['input', 'underline']. This will cause a prop validation error in development mode and potentially unexpected behavior in production.
Apply this diff to fix the validation:
inputBoxType: {
type: String,
default: 'normal',
- validator: (value: string) => ['input', 'underline'].includes(value)
+ validator: (value: string) => ['normal', 'input', 'underline'].includes(value)
},Alternatively, if 'normal' should not be a valid value, change the default back to 'input' to maintain backward compatibility.
📝 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.
| inputBoxType: { | |
| type: String, | |
| default: 'input', | |
| default: 'normal', | |
| validator: (value: string) => ['input', 'underline'].includes(value) | |
| }, | |
| inputBoxType: { | |
| type: String, | |
| default: 'normal', | |
| validator: (value: string) => ['normal', 'input', 'underline'].includes(value) | |
| }, |
🤖 Prompt for AI Agents
In packages/vue/src/select-wrapper/src/index.ts around lines 329 to 333, the
prop inputBoxType currently defaults to 'normal' while its validator only allows
['input','underline'], causing validation errors; fix by setting the default to
'input' to match the validator (or, if 'normal' is intended, add 'normal' to the
validator array) and ensure the default value and validator remain consistent.
1 participant
PR
feat:select-wrapper多端开发
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.