fix(picker): add popperOptions props for Picker #3769
Conversation
WalkthroughAdds a new popper options interface (IPopperOption) and corresponding public prop to multiple demo API files, and introduces a popperOptions prop to the core picker props. No control-flow logic changes are described; updates are type and prop additions for configuring popper behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as App Component
participant UI as Picker/Select/Date/Time Component
participant Core as Base Picker (pickerProps)
participant Popper as Popper Engine
User->>App: Interacts with picker
App->>UI: Pass props (including popper-options / popperOptions)
UI->>Core: Forward popperOptions (object)
Core->>Popper: Initialize/Update with popperOptions
Note right of Popper: Uses boundariesElement, bubbling, etc.
Popper-->>Core: Positioned overlay
Core-->>UI: Render popup per options
UI-->>User: Display dropdown/calendar
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
♻️ Duplicate comments (3)
examples/sites/demos/apis/select.js (1)
1051-1065: Interface duplication detected.This
IPopperOptioninterface is identical to the one defined intime-picker.js(lines 400-414),time-select.js(lines 218-234), anddate-picker.js(lines 683-697). As mentioned in the review oftime-picker.js, please consolidate this interface into a shared types file to eliminate duplication and make the properties optional to match the documented defaults.examples/sites/demos/apis/time-select.js (1)
218-234: Interface duplication detected.This
IPopperOptioninterface duplicates definitions intime-picker.js,select.js, anddate-picker.js. Please refer to the recommendations in thetime-picker.jsreview to consolidate this interface and make properties optional.examples/sites/demos/apis/date-picker.js (1)
683-697: Interface duplication detected.This is the fourth instance of the identical
IPopperOptioninterface across the demo API files. As recommended in previous comments, consolidate this into a shared type definition and make all properties optional.
🧹 Nitpick comments (4)
examples/sites/demos/apis/time-picker.js (1)
165-177: Provide demo examples forpopper-optionsand normalize its defaultValue formatting
- In examples/sites/demos/apis/time-picker.js,
pcDemoandmfDemoare both empty; add demo entries (demoId + code files) to show how to configure IPopperOption for TimePicker.- Change
defaultValue: ' { }'todefaultValue: '{}'for consistency with other prop definitions.examples/sites/demos/apis/select.js (1)
438-450: Add Popper options demos for Select. BothpcDemoandmfDemoin examples/sites/demos/apis/select.js are empty; please add demo IDs illustratingpopper-optionsusage for PC and mobile-first.examples/sites/demos/apis/time-select.js (1)
131-143: Add demo examples for popper-options
BothpcDemoandmfDemoare empty in examples/sites/demos/apis/time-select.js and no usage ofpopper-optionsappears elsewhere; add example code or remove these empty fields.examples/sites/demos/apis/date-picker.js (1)
218-230: Populate date-picker’spopper-optionsdemos
In examples/sites/demos/apis/date-picker.js thepcDemoandmfDemofields forpopper-optionsare empty; add and reference demo files (e.g.popper-options.vue) for both PC and mobile-first modes, mirroring other components’ demos.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/sites/demos/apis/date-picker.js(2 hunks)examples/sites/demos/apis/select.js(2 hunks)examples/sites/demos/apis/time-picker.js(2 hunks)examples/sites/demos/apis/time-select.js(2 hunks)packages/vue/src/picker/src/type.ts(1 hunks)
⏰ 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)
| { | ||
| name: 'IPopperOption', | ||
| type: 'interface', | ||
| code: ` | ||
| interface IPopperOption { | ||
| bubbling: boolean // 是否监听元素所有上级有滚动元素的scroll事件，监听到则更新popper的位置。用于解决某些弹出层位置在页面滚动时，位置不正确的场景，默认false | ||
| followReferenceHide: boolean // 当触发源隐藏时，自动隐藏弹出层，默认true | ||
| removeOnDestroy: boolean // 弹出层消失后，是否移除弹出层的DOM元素，布尔false | ||
| updateHiddenPopperOnScroll: boolean // 滚动过程中是否更新隐藏的弹出层位置 | ||
| boundariesElement: 'viewport' | 'body' | HTMLElement // 滚动过程中,弹出层的碰撞边界。 默认值为： 'viewport' | ||
| ignoreBoundaries: boolean // 忽略边界判断，弹出的位置始终是设置的 placement 值 | ||
| scrollParent: HTMLElement // 指定滚动的父节点，优化级最高。 默认为null | ||
| } | ||
| ` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate duplicated IPopperOption interface and make properties optional.
The IPopperOption interface is duplicated across multiple files (date-picker.js, select.js, time-select.js, and here). This violates the DRY principle and increases maintenance burden.
Additionally, all properties are defined as required, but the comments indicate default values (e.g., "默认false", "默认true", "默认为null"), suggesting these should be optional properties.
Consider:
- Create a shared type definition: Move
IPopperOptionto a common types file (e.g.,examples/sites/demos/apis/types/popper.ts) and import it across all demo API files. - Make properties optional: Update the interface to use optional markers:
interface IPopperOption {
- bubbling: boolean // 是否监听元素所有上级有滚动元素的scroll事件,监听到则更新popper的位置。用于解决某些弹出层位置在页面滚动时,位置不正确的场景,默认false
- followReferenceHide: boolean // 当触发源隐藏时,自动隐藏弹出层,默认true
- removeOnDestroy: boolean // 弹出层消失后,是否移除弹出层的DOM元素,布尔false
- updateHiddenPopperOnScroll: boolean // 滚动过程中是否更新隐藏的弹出层位置
- boundariesElement: 'viewport' | 'body' | HTMLElement // 滚动过程中,弹出层的碰撞边界。 默认值为: 'viewport'
- ignoreBoundaries: boolean // 忽略边界判断,弹出的位置始终是设置的 placement 值
- scrollParent: HTMLElement // 指定滚动的父节点,优化级最高。 默认为null
+ bubbling?: boolean // 是否监听元素所有上级有滚动元素的scroll事件,监听到则更新popper的位置。用于解决某些弹出层位置在页面滚动时,位置不正确的场景,默认false
+ followReferenceHide?: boolean // 当触发源隐藏时,自动隐藏弹出层,默认true
+ removeOnDestroy?: boolean // 弹出层消失后,是否移除弹出层的DOM元素,布尔false
+ updateHiddenPopperOnScroll?: boolean // 滚动过程中是否更新隐藏的弹出层位置
+ boundariesElement?: 'viewport' | 'body' | HTMLElement // 滚动过程中,弹出层的碰撞边界。 默认值为: 'viewport'
+ ignoreBoundaries?: boolean // 忽略边界判断,弹出的位置始终是设置的 placement 值
+ scrollParent?: HTMLElement // 指定滚动的父节点,优化级最高。 默认为null
}📝 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.
| { | |
| name: 'IPopperOption', | |
| type: 'interface', | |
| code: ` | |
| interface IPopperOption { | |
| bubbling: boolean // 是否监听元素所有上级有滚动元素的scroll事件，监听到则更新popper的位置。用于解决某些弹出层位置在页面滚动时，位置不正确的场景，默认false | |
| followReferenceHide: boolean // 当触发源隐藏时，自动隐藏弹出层，默认true | |
| removeOnDestroy: boolean // 弹出层消失后，是否移除弹出层的DOM元素，布尔false | |
| updateHiddenPopperOnScroll: boolean // 滚动过程中是否更新隐藏的弹出层位置 | |
| boundariesElement: 'viewport' | 'body' | HTMLElement // 滚动过程中,弹出层的碰撞边界。 默认值为： 'viewport' | |
| ignoreBoundaries: boolean // 忽略边界判断，弹出的位置始终是设置的 placement 值 | |
| scrollParent: HTMLElement // 指定滚动的父节点，优化级最高。 默认为null | |
| } | |
| ` | |
| } | |
| { | |
| name: 'IPopperOption', | |
| type: 'interface', | |
| code: ` | |
| interface IPopperOption { | |
| bubbling?: boolean // 是否监听元素所有上级有滚动元素的scroll事件，监听到则更新popper的位置。用于解决某些弹出层位置在页面滚动时，位置不正确的场景，默认false | |
| followReferenceHide?: boolean // 当触发源隐藏时，自动隐藏弹出层，默认true | |
| removeOnDestroy?: boolean // 弹出层消失后，是否移除弹出层的DOM元素，布尔false | |
| updateHiddenPopperOnScroll?: boolean // 滚动过程中是否更新隐藏的弹出层位置 | |
| boundariesElement?: 'viewport' | 'body' | HTMLElement // 滚动过程中,弹出层的碰撞边界。 默认值为： 'viewport' | |
| ignoreBoundaries?: boolean // 忽略边界判断，弹出的位置始终是设置的 placement 值 | |
| scrollParent?: HTMLElement // 指定滚动的父节点，优化级最高。 默认为null | |
| } | |
| ` | |
| } |
🤖 Prompt for AI Agents
In examples/sites/demos/apis/time-picker.js around lines 400 to 414, the
IPopperOption interface is duplicated and declares all properties as required
despite comments indicating defaults; extract this interface into a shared
module (e.g., examples/sites/demos/apis/types/popper.ts), change each property
to optional (use ?), export the interface from that file, replace the inline
definition here with an import from the new shared types file, and update the
other files (date-picker.js, select.js, time-select.js) to import the same
shared IPopperOption to avoid duplication and ensure default/optional semantics
are correct.
| popperOptions: { | ||
| type: Object, | ||
| default: () => ({}) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Improve type safety for popperOptions prop.
The popperOptions prop currently uses the generic Object type, which provides no type checking or IntelliSense support for consumers. Since a well-defined IPopperOption interface exists in the demo API files, consider creating a proper TypeScript interface and using it here.
Recommendation:
- Create a shared TypeScript type definition file (e.g.,
packages/vue/src/picker/src/popper-types.ts):
export interface IPopperOption {
bubbling?: boolean
followReferenceHide?: boolean
removeOnDestroy?: boolean
updateHiddenPopperOnScroll?: boolean
boundariesElement?: 'viewport' | 'body' | HTMLElement
ignoreBoundaries?: boolean
scrollParent?: HTMLElement
}- Import and use the interface in this file:
+import type { IPopperOption } from './popper-types'
export const pickerProps = {
// ...
popperOptions: {
- type: Object,
+ type: Object as PropType<IPopperOption>,
default: () => ({})
},This change would provide better developer experience through:
- Type checking at compile time
- IDE autocomplete for configuration options
- Better documentation through types
🤖 Prompt for AI Agents
In packages/vue/src/picker/src/type.ts around lines 78 to 81 the popperOptions
prop is typed as generic Object; create a proper interface and use it instead:
add a new file packages/vue/src/picker/src/popper-types.ts exporting
IPopperOption with the suggested fields (bubbling, followReferenceHide,
removeOnDestroy, updateHiddenPopperOnScroll, boundariesElement,
ignoreBoundaries, scrollParent), then import IPopperOption into type.ts and
change the prop declaration to use that interface (and update the default to
return an empty object typed as IPopperOption) so consumers get compile-time
type checking and IDE autocomplete.
PR
为picker组件添加 popperOptions属性
为select, datepicker, timepicker,timeselect等补充 popperOptions属性的api文档
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
Documentation