feat(dropdown): [dropdown] Add visible attribute, support user-defined panel display and hide #2827
Conversation
WalkthroughThis pull request introduces a new feature to the dropdown component by adding a Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| await page.goto('dropdown#visible') | ||
|
|
||
| const wrap = page.locator('#visible') | ||
| const dropDownMenu = page.locator('body > .tiny-dropdown-menu').locator('visible=true') |
There was a problem hiding this comment.
The locator body > .tiny-dropdown-menu with visible=true might not work as expected if the visible attribute is not directly reflected in the DOM. Consider verifying the visibility through a different method, such as checking the CSS display property or using a specific class that indicates visibility.
WalkthroughThis pull request introduces enhanced visibility control for the dropdown component across multiple files. The primary change is the addition of a new Changes
Possibly related PRs
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 (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 3
🧹 Nitpick comments (5)
examples/sites/demos/pc/app/dropdown/visible.spec.ts (1)
3-27: Enhance test coverage with additional scenarios.Consider adding test cases for:
- Button text changes based on visibility state
- Error cases (e.g., programmatically setting visible when disabled)
- Edge cases (e.g., rapid toggling of visibility)
examples/sites/demos/pc/app/dropdown/visible-composition-api.vue (2)
23-39: Extract shared options array to avoid duplication.The options array is duplicated between this file and
visible.vue. Consider extracting it to a shared constants file.
40-47: Add TypeScript type definitions for event handlers and data.Consider adding type definitions for:
- The
itemClickevent handler parameter- The options array items
Example:
interface DropdownItem { label: string disabled?: boolean } const options: DropdownItem[] = [...] const itemClick = (e: { itemData: DropdownItem }) => { if (e.itemData.label === '点击我隐藏') { visible.value = false } }examples/sites/demos/pc/app/dropdown/visible.vue (1)
1-61: Consider using Composition API for consistency.The codebase has two similar components with different APIs (Options vs Composition). Consider using Composition API consistently across components for better maintainability.
Additionally, extract shared code (options array, click handlers) to reduce duplication with
visible-composition-api.vue.examples/sites/demos/apis/dropdown.js (1)
221-231: LGTM! API documentation is complete and accurate.The
v-model:visibleproperty is well-documented with:
- Correct Vue v-model syntax
- Appropriate boolean type and default value
- Clear description of functionality and priority
- Proper demo reference
However, consider adding this property to the mobile-first mode as well, since visibility control would be useful for mobile interfaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
examples/sites/demos/apis/dropdown.js(1 hunks)examples/sites/demos/pc/app/dropdown/visible-composition-api.vue(1 hunks)examples/sites/demos/pc/app/dropdown/visible.spec.ts(1 hunks)examples/sites/demos/pc/app/dropdown/visible.vue(1 hunks)examples/sites/demos/pc/app/dropdown/webdoc/dropdown.js(1 hunks)packages/renderless/package.json(2 hunks)packages/renderless/src/dropdown/index.ts(6 hunks)packages/renderless/src/dropdown/vue.ts(3 hunks)packages/renderless/types/dropdown.type.ts(1 hunks)packages/vue/src/dropdown/package.json(1 hunks)packages/vue/src/dropdown/src/index.ts(1 hunks)packages/vue/src/dropdown/src/pc.vue(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/renderless/package.json
- packages/vue/src/dropdown/package.json
🔇 Additional comments (11)
examples/sites/demos/pc/app/dropdown/visible.spec.ts (1)
8-8: Consider using a more reliable locator strategy.The locator
body > .tiny-dropdown-menuwithvisible=truemight not work as expected if thevisibleattribute is not directly reflected in the DOM. Consider verifying the visibility through a different method, such as checking the CSS display property or using a specific class that indicates visibility.packages/renderless/types/dropdown.type.ts (2)
23-23: LGTM! Type enhancement for menuItems.The addition of empty array as a valid type improves type safety by explicitly handling the empty state scenario.
31-32: LGTM! Well-defined types for new properties.The types are correctly defined:
triggeris properly constrained to literal types 'click' | 'hover'visibleIsBooleanis appropriately typed as booleanpackages/vue/src/dropdown/src/pc.vue (2)
109-110: LGTM! Proper v-model event declaration.The 'update:visible' event is correctly added to support the v-model:visible binding.
219-219: LGTM! Conditional v-clickoutside binding.The v-clickoutside directive is correctly conditioned on !state.visibleIsBoolean to prevent conflicts with external visibility control.
packages/renderless/src/dropdown/vue.ts (2)
65-66: LGTM! Well-implemented computed property.The visibleIsBoolean computed property correctly determines if visibility is externally controlled.
95-99: LGTM! Proper visibility watch setup.The watch setup correctly handles both internal and external visibility control:
- Watches props.visible when externally controlled
- Falls back to watching state.visible for internal control
packages/renderless/src/dropdown/index.ts (2)
213-215: LGTM! Proper event handling separation.The initEvent function correctly skips mouse event registration when visibility is externally controlled.
280-282: LGTM! Conditional event listener registration.The is-disabled event listener is correctly registered only when visibility is internally controlled.
examples/sites/demos/pc/app/dropdown/webdoc/dropdown.js (1)
114-126: LGTM! Well-documented demo entry.The demo entry is properly structured with clear descriptions in both languages, accurately explaining the
visibleattribute's functionality and its priority over thetriggerattribute.examples/sites/demos/apis/dropdown.js (1)
221-231: Verify the implementation consistency.Let's verify that the visible property is properly implemented in the component and doesn't introduce breaking changes.
✅ Verification successful
Implementation verified: v-model:visible prop is correctly implemented
The dropdown component properly implements the v-model:visible binding with correct two-way data flow and proper interaction with the trigger prop, matching the API documentation. The implementation allows manual control of the dropdown's visibility state while maintaining compatibility with trigger-based showing/hiding.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing usage of visible prop in components and tests echo "Checking for existing usage of visible prop..." rg -l "v-model:visible" --type vue # Check for potential conflicts with existing visible props echo "Checking for potential conflicts..." rg "(?:props.*visible|visible.*props)" --type vue # Check for existing trigger-related logic that might be affected echo "Checking trigger-related logic..." rg "trigger\s*[=:].*['\"](?:hover|click)['\"]" --type vueLength of output: 495
Script:
#!/bin/bash # Find dropdown-related files echo "Finding dropdown-related files..." fd -t f "dropdown" -E "*.md" # Check for visible prop usage echo "Checking visible prop usage..." rg "v-model:visible|:visible=|visible:" -l # Check for trigger-related logic echo "Checking trigger implementation..." rg "trigger.*(?:hover|click)" -l # Look for dropdown component definition echo "Looking for dropdown component definition..." ast-grep --pattern 'export default { name: "Dropdown", $$$ }'Length of output: 46140
Script:
#!/bin/bash # Check dropdown component implementation echo "Checking dropdown component implementation..." cat packages/vue/src/dropdown/src/pc.vue packages/vue/src/dropdown/src/mobile-first.vue # Check visible demo implementation echo "Checking visible demo..." cat examples/sites/demos/pc/app/dropdown/visible.vue # Check dropdown type definitions echo "Checking dropdown type definitions..." cat packages/renderless/types/dropdown.type.tsLength of output: 17054
| visible: { | ||
| type: Boolean || undefined, | ||
| default: undefined | ||
| }, |
There was a problem hiding this comment.
Fix the TypeScript type definition for the visible prop.
The current type definition Boolean || undefined is incorrect TypeScript syntax.
Apply this diff to fix the type definition:
- visible: {
- type: Boolean || undefined,
- default: undefined
- },
+ visible: {
+ type: Boolean,
+ default: undefined
+ },📝 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.
| visible: { | |
| type: Boolean || undefined, | |
| default: undefined | |
| }, | |
| visible: { | |
| type: Boolean, | |
| default: undefined | |
| }, |
| // tiny新增 | ||
| visible: { | ||
| type: Boolean || undefined, | ||
| default: undefined | ||
| }, |
There was a problem hiding this comment.
Fix incorrect prop type definition.
The visible prop type is incorrectly defined. In Vue, you cannot use the OR operator for prop types.
Apply this diff to fix the prop type:
- visible: {
- type: Boolean || undefined,
- default: undefined
- },
+ visible: {
+ type: Boolean,
+ default: undefined
+ },📝 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.
| // tiny新增 | |
| visible: { | |
| type: Boolean || undefined, | |
| default: undefined | |
| }, | |
| // tiny新增 | |
| visible: { | |
| type: Boolean, | |
| default: undefined | |
| }, |
| if (state.visibleIsBoolean) { | ||
| emit('update:visible', true) | ||
| } else { | ||
| clearTimeout(Number(state.timeout)) | ||
|
|
||
| state.timeout = setTimeout( | ||
| () => { | ||
| state.visible = true | ||
| }, | ||
| state.trigger === 'click' ? 0 : props.showTimeout | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential race condition in show timeout handling.
The timeout is cleared but there's a potential race condition if show/hide are called in quick succession.
Consider using a state flag to track pending timeouts:
+ if (state.timeout) {
+ clearTimeout(Number(state.timeout))
+ state.timeout = null
+ }Committable suggestion skipped: line range outside the PR's diff.
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
Release Notes
New Features
v-model:visibleproperty.visibleandmenuOptions.Documentation
Bug Fixes
Version Updates