feat(base-select): [base-select] Synchronize the select component code to the base-select component #3834
Conversation
WalkthroughMobile-first and searchable-aware behavior was added to base-select: public functions now accept designConfig and isMobileFirstMode, new exports and state (currentSizeMap, auto-select watcher, onMouseenterSelf) were introduced, component props were extended, icons/ref bindings refactored, and tests updated to use input-root click triggers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VueComponent
participant RenderlessAPI
participant State
User->>VueComponent: click / type / hover
VueComponent->>RenderlessAPI: toggleMenu(e, designConfig)
RenderlessAPI->>State: evaluate isMobileFirstMode + (filterable || searchable)
alt mobile-first
RenderlessAPI->>VueComponent: early-return or mobile UI flow
else desktop
RenderlessAPI->>State: normal visibility/focus logic
end
RenderlessAPI->>State: update model, emitChange(isMobileFirstMode)
State-->>VueComponent: updated props/state
VueComponent->>User: render updated UI
sequenceDiagram
participant OptionsWatcher
participant API
participant Component
OptionsWatcher->>API: options changed (remote)
alt autoSelect enabled
API->>Component: auto-update modelValue
Component->>Component: re-render selection
else autoSelect disabled
OptionsWatcher-->>Component: no auto-selection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
⏰ 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)
🔇 Additional comments (6)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/renderless/src/base-select/vue.ts (1)
361-377: PassdesignConfigintotoggleMenuinstead ofisMobileFirstModeIn
initApi,toggleMenuis currently initialized as:toggleMenu: toggleMenu({ vm, state, props, api, isMobileFirstMode }),but in
packages/renderless/src/base-select/index.tsthe signature is:export const toggleMenu = ({ vm, state, props, api, designConfig }) => (e) => { ... }Inside
toggleMenu,designConfigis used (e.g.designConfig?.props?.stopPropagation), so passingisMobileFirstModehere leavesdesignConfigundefinedand effectively disables the design-levelstopPropagationdefault.Update the call to:
- toggleMenu: toggleMenu({ vm, state, props, api, isMobileFirstMode }), + toggleMenu: toggleMenu({ vm, state, props, api, designConfig }),to align with the new signature and make
stopPropagationwork withdesignConfig.packages/renderless/src/base-select/index.ts (1)
365-408: Bug intoggleCheckAllfiltered-uncheck branch (value overwritten to union)In the filtered “select all” case:
if (filtered) { if (state.filteredSelectCls === 'check' || state.filteredSelectCls === 'halfselect') { value = [...new Set([...state.modelValue, ...enabledValues])] } else { value = state.modelValue.filter((val) => !enabledValues.includes(val)) // 避免编译报错 value = Array.from(new Set([...state.modelValue, ...enabledValues])) } }The
elsebranch is supposed to uncheck all currently enabled filtered options (i.e. removeenabledValuesfromstate.modelValue). However,valueis immediately overwritten with the unionnew Set([...state.modelValue, ...enabledValues]), so the “uncheck” operation never happens—no values are removed.This breaks the UX for toggling “全选(筛选后)” off.
A minimal fix is to remove the second assignment:
- } else { - value = state.modelValue.filter((val) => !enabledValues.includes(val)) - // 避免编译报错 - value = Array.from(new Set([...state.modelValue, ...enabledValues])) - } + } else { + // When already fully selected, uncheck the filtered items + value = state.modelValue.filter((val) => !enabledValues.includes(val)) + }If the original comment was addressing a specific compile-time issue, we should tackle that differently (e.g. ensuring
valueis always assigned before use) rather than reassigning it to the opposite value.
🧹 Nitpick comments (4)
packages/vue/src/select/src/pc.vue (1)
389-396: Use$eventin@update:modelValueinstead ofstate.queryRelying on
state.queryinside the@update:modelValuehandler depends on Vue’s internal handler ordering withv-model. It’s safer and clearer to use the emitted value:- @update:modelValue="handleQueryChange(state.query, false, true)" + @update:modelValue="(val) => handleQueryChange(val, false, true)"This avoids any timing issues between the v-model assignment and the handler execution.
packages/renderless/src/base-select/vue.ts (1)
568-589: Auto-select watcher wiring is reasonable but only reacts to array reference changesThe new watchers:
watch(() => state.options, api.watchOptionsWhenAutoSelect) props.options && watch(() => props.options, api.watchOptionsWhenAutoSelect)will only fire when
state.options/props.optionsreferences change, not when items are pushed/spliced in-place. If remote data is typically replaced (this.options = [...]) this is fine; if it is mutated in-place,autoSelectmay never trigger.If in-place mutation is common in this codebase, consider mirroring
watchOptionsand using a shallow copy:- watch(() => state.options, api.watchOptionsWhenAutoSelect) + watch(() => [...state.options], api.watchOptionsWhenAutoSelect)(and similarly for
props.optionsif needed).packages/vue/src/base-select/src/pc.vue (2)
389-396: Align@update:modelValuehandler with emitted valueSame as in
select/src/pc.vue, this searchable input uses:@update:modelValue="handleQueryChange(state.query, false, true)"This couples behavior to the v-model update order. Prefer using the emitted value:
- @update:modelValue="handleQueryChange(state.query, false, true)" + @update:modelValue="(val) => handleQueryChange(val, false, true)"for clearer and safer behavior.
471-477: Loading block always renders spinner component, even when not loadingIn the empty/loading section:
<div v-else class="tiny-select-dropdown__loading" :class="{ 'show-loading-icon': loading }"> <template v-if="!loading"> <!-- empty image/text --> </template> <component class="circular" :is="(state.designConfig && state.designConfig.icons && state.designConfig.icons.loadingIcon) || 'icon-loading-shadow'" ></component> </div>the spinner
<component>is rendered regardless ofloading. Inselect/src/pc.vue, the spinner is guarded withv-if="loading"and the empty text is inside thev-elsebranch, which avoids rendering the spinner when not loading.For visual and behavioral consistency, consider:
- <div v-else class="tiny-select-dropdown__loading" :class="{ 'show-loading-icon': loading }"> - <template v-if="!loading"> - <span v-if="showEmptyImage" class="tiny-select-dropdown__empty-images"></span> - <span v-else class="tiny-select-dropdown__empty"> {{ state.emptyText }}</span> - </template> - <component - class="circular" - :is=" - (state.designConfig && state.designConfig.icons && state.designConfig.icons.loadingIcon) || - 'icon-loading-shadow' - " - ></component> - </div> + <div v-else class="tiny-select-dropdown__loading" :class="{ 'show-loading-icon': loading }"> + <component + v-if="loading" + class="circular" + :is=" + (state.designConfig && state.designConfig.icons && state.designConfig.icons.loadingIcon) || + 'icon-loading-shadow' + " + ></component> + <template v-else> + <span v-if="showEmptyImage" class="tiny-select-dropdown__empty-images"></span> + <span v-else class="tiny-select-dropdown__empty"> {{ state.emptyText }}</span> + </template> + </div>to match the select component and avoid showing (or needing to hide) a spinner when not loading.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/renderless/src/base-select/index.ts(36 hunks)packages/renderless/src/base-select/vue.ts(13 hunks)packages/vue/src/base-select/src/index.ts(3 hunks)packages/vue/src/base-select/src/pc.vue(20 hunks)packages/vue/src/select/src/pc.vue(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/vue/src/base-select/src/index.tspackages/vue/src/base-select/src/pc.vue
🧬 Code graph analysis (2)
packages/renderless/src/base-select/vue.ts (2)
packages/vue-hooks/src/useUserAgent.ts (1)
useUserAgent(15-18)packages/renderless/src/base-select/index.ts (10)
emitChange(185-193)directEmitChange(195-201)toggleMenu(761-795)setSoftFocus(718-736)onMouseenterSelf(1502-1509)updateModelValue(1827-1837)computedReadonly(1884-1898)computedShowNewOption(1913-1923)computedCurrentSizeMap(1676-1683)watchOptionsWhenAutoSelect(1365-1378)
packages/renderless/src/base-select/index.ts (1)
packages/renderless/src/option/index.ts (1)
escapeRegexpString(15-15)
⏰ 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 (17)
packages/renderless/src/base-select/vue.ts (1)
167-283: New state fields for display-only content, sizing, and AutoTip look consistentThe additions in
initState/initStateAdd(isIOS,displayOnlyContent,currentSizeMap,rootAutoTipConfig, focus timers, etc.) are coherent with the renderless/index.ts exports and vue-layer usage (e.g. AutoTip and display-only tooltip behavior). No functional issues spotted in this block.packages/vue/src/base-select/src/pc.vue (5)
78-115: Tag tooltip and width enhancements look correctUsing
isTagClosable,state.isDisabled,maxTagWidth, andtooltipConfig(effect/placement/popper-class) on the collapsed first tag is consistent with the renderless API and mirrors the select component. No issues spotted with these bindings.
219-225: Display-only multi-select text now honorsstate.isDisabledThe updated display-only span correctly adds the
'is-disabled'class based onstate.isDisabled, matching the root disabled semantics. This is a good alignment with the common disabled handling.
260-285: tiny-input flags and classes now consistent with extended propsThe additions:
:showTooltip="false":show-empty-value="showEmptyValue"- class flags
'is-show-close','show-copy','show-clear'are consistent with the new props from
base-select/index.tsand with the select component’s behavior. No functional problems here.
569-651: Directive and icon registration matches new renderless and icon APIsAdding
AutoTiptodirectivesand switching icons to theicon*()factories (IconClose, IconCopy, IconHalfselect, IconCheck, IconCheckedSur, IconSearch, IconDownWard, IconEllipsis, IconChevronUp) is consistent with the updated renderless/base-select and icon packages. Component registration looks correct and complete.
724-742: New props exposure (initLabel, tooltipConfig, showEmptyValue, stopPropagation, allText, maxTagWidth) is aligned with base-select APIThese props mirror the definitions in
packages/vue/src/base-select/src/index.tsand are all actually consumed in the template or renderless logic (e.g. initLabel/initLabel handling, tooltipConfig, showEmptyValue, stopPropagation in toggleMenu, allText and maxTagWidth in tags). No issues with the public API surface here.packages/renderless/src/base-select/index.ts (11)
83-94: Regex escaping inqueryChangecorrectly handles special charactersUsing
escapeRegexpStringin:const filterDatas = state.initDatas.filter((item) => new RegExp(escapeRegexpString(value), 'i').test(item[props.textField]) )is a good hardening against user-entered regex metacharacters causing errors or unintended matches. This looks correct and safe.
120-162:handleQueryChangeextension tosearchableand disabled checks is soundThe updated guard:
if ( props.multiple && (props.filterable || props.searchable) && !props.shape && !state.selectDisabled ) { nextTick(() => { const length = vm.$refs.input.value.length * 15 + 20 state.inputLength = state.collapseTags ? Math.min(50, length) : length api.managePlaceholder() api.resetInputHeight() }) }correctly generalizes from
filterableto(filterable || searchable)and respectsstate.selectDisabledso we don’t mutate input geometry when disabled/display-only. No issues here.
185-193: Mobile-first change emission guard (emitChange) looks correctThe new
emitChangewrapper:export const emitChange = ({ emit, props, state, constants, isMobileFirstMode }) => (value, changed) => { if (isMobileFirstMode && state.device === 'mb' && props.multiple && !changed) return if (!isEqual(props.modelValue, state.compareValue)) { emit('change', value) } }matches the mobile-first requirement to avoid emitting redundant
changeevents in mobile multiple-select mode unless explicitly marked aschanged. The comparison vsstate.compareValuepreserves previous behavior; the early-return condition is scoped tightly enough to not affect non-mobile or single-select flows.
561-615: Resetting input height withcurrentSizeMapand spacing is consistentThe updated
resetInputHeight:
- Skips when
collapseTagsis on and neitherfilterablenorsearchableare enabled.- Uses
spacingHeightfromdesignConfigwith a sane default.- Bases height on
Math.max(tagsClientHeight + spacingHeight, state.currentSizeMap)orstate.currentSizeMapwhen no selection.Together with
computedCurrentSizeMapand thedesignConfig.state.sizeMapdefault fallbacks, this provides predictable sizing across themes and is aligned with the new sizing API. No issues detected.
1199-1229:postOperOfToVisiblelogic forinitLabeland searchable is coherentThe new logic:
- For multiple:
- When
props.modelValueandprops.initLabelare set butstate.selectedis empty, it usesinitLabelasselectedLabel.- For single:
- Honors grid/tree vs normal renderType when setting
selectedLabel.- Handles
allowCreate+createdSelectedfor filterable/searchable.- Keeps
state.queryin sync when(filterable || searchable)is true.- Falls back to
initLabelwhen modelValue is present butselectedLabelis empty.This seems consistent with the new
initLabelprop semantics and with searchable/filterable behavior.
1291-1333:watchVisibleenhancements for searchable/mobile-first look correctKey additions:
- Blur reference input on close when
(filterable || searchable || remote)to avoid lingering focus.- In mobile-first multiple mode, copy
state.selectedintostate.selectedCopyon open for staging.- Keep existing updatePopper/scrollbar logic and
shape === 'filter'softFocus reset.These changes are coherent with the broader mobile-first design and do not introduce obvious regressions.
1650-1674:initQuerycorrectly generalizes to(filterable || searchable)for remote initializationThe
isRemoteflag:const isRemote = (props.filterable || props.searchable) && props.remote && (typeof props.remoteMethod === 'function' || typeof props.initQuery === 'function')properly includes
searchablealongsidefilterablefor remote initialization while preserving the original function-type checks. The promise handling for asyncinitQueryalso looks correct.
1676-1683:computedCurrentSizeMapand its use in sizing is well-structured
computedCurrentSizeMap:export const computedCurrentSizeMap = ({ state, designConfig }) => () => { const defaultSizeMap = { default: 32, mini: 24, small: 28, medium: 40 } const sizeMap = designConfig?.state?.sizeMap || defaultSizeMap return sizeMap[state.selectSize || 'default'] }provides a clean extension point for theme-driven sizing via
designConfig.state.sizeMapwith sensible defaults and integrates neatly withresetInputHeight.
1884-1898:computedReadonly’s iOS/mobile-first/searchable logic is coherentThe new implementation:
- Special-cases iOS +
props.filterableto always returnfalse(input editable), working around platform quirks.- Otherwise returns
truewhen:
- mobile-first + device is mb, or
props.readonly, or- neither
filterablenorsearchableare enabled, orprops.multiple, or- modern browsers with dropdown closed.
This looks intentional and consistent with the mobile-first and searchable requirements.
1933-1945:computedDisabledTooltipContentandcomputedSelectDisabledalign with display-only semanticsThese computations:
export const computedDisabledTooltipContent = ({ state }) => () => state.displayOnlyContent export const computedSelectDisabled = ({ state }) => () => state.isDisabled || state.isDisplayOnlytie the disabled tooltip text directly to the display-only content and centralize the notion of “select-disabled” as either disabled or display-only. This matches the new
displayOnlyContentandisDisabledstate fields and simplifies template logic.
2046-2049:handleDebouncedQueryChangecorrectly passesisInput=trueChanging the debounce callback to:
debounce(state.debounce, (value) => { api.handleQueryChange(value, false, true) })ensures
isInputis correctly set when debounced queries are triggered, which is important for the optimization path that only runs whenisInputis true.
…e to the base-select component
wuyiping0628
changed the title
PR
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.