feat(dropdown): [dropdown] add right side expansion function for dropdown #2608
Conversation
WalkthroughThe pull request introduces significant modifications to the dropdown components in the project. Changes include updates to property types and the addition of new properties in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)examples/sites/demos/apis/dropdown.js (2)
The experimental tag and demo link are properly added for the new placement feature.
The modification of Recommendations:
Alternative implementation that maintains compatibility: -type IPlacementType = 'bottom-start' | 'bottom-end'
+type IPlacementType = 'top' | 'top-start' | 'top-end' | 'bottom-start' | 'bottom-end'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 (
|
|
|
|
MomoPoppy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
examples/sites/demos/pc/app/dropdown/placement-composition-api.vue (1)
23-50: Add JSDoc comments to document component purposeThe script section lacks documentation explaining the purpose of this demo and how it relates to the new right-side expansion feature.
Add descriptive comments:
+/** + * @description Demonstrates dropdown placement and expansion options + * Shows: + * - Different placement options (bottom-start, bottom-end) + * - Right-side expansion behavior + * - Static and dynamic dropdown content + */ <script setup>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
examples/sites/demos/apis/dropdown.js(1 hunks)examples/sites/demos/pc/app/dropdown/placement-composition-api.vue(1 hunks)examples/sites/demos/pc/app/dropdown/placement.vue(1 hunks)examples/sites/demos/pc/app/dropdown/webdoc/dropdown.js(1 hunks)packages/theme/src/dropdown-item/index.less(3 hunks)packages/theme/src/dropdown-item/vars.less(1 hunks)
🔇 Additional comments (4)
examples/sites/demos/pc/app/dropdown/placement.vue (1)
1-60:
Maintain consistency with Composition API version
This Options API version should be updated to match the changes recommended for the Composition API version, including:
- Demonstration of right-side expansion
- Documentation comments
- Additional placement examples
packages/theme/src/dropdown-item/index.less (2)
72-72: LGTM: Consistent variable renaming for bi-directional support
The margin variable renaming from -margin-right to -margin-x better reflects their bi-directional usage, supporting both left and right expansion modes.
Also applies to: 91-91
158-202: LGTM: Well-structured right-side expansion styles
The implementation for right-side expansion is well thought out:
- Uses
flex-direction: row-reversefor layout - Properly handles icon transformations
- Correctly positions child dropdowns using
left: 100%
examples/sites/demos/pc/app/dropdown/webdoc/dropdown.js (1)
30-43: LGTM: Clear and complete documentation for placement feature
The new demo entry is well-documented:
- Clear descriptions in both Chinese and English
- Properly explains the
bottom-startplacement value - References the correct example file
| <template> | ||
| <tiny-dropdown> | ||
| <template #dropdown> | ||
| <tiny-dropdown-menu placement="bottom-start"> | ||
| <tiny-dropdown-item label="老友粉"></tiny-dropdown-item> | ||
| <tiny-dropdown-item>黄金糕</tiny-dropdown-item> | ||
| <tiny-dropdown-item>狮子头</tiny-dropdown-item> | ||
| <tiny-dropdown-item>螺蛳粉</tiny-dropdown-item> | ||
| <tiny-dropdown-item disabled>双皮奶</tiny-dropdown-item> | ||
| <tiny-dropdown-item>蚵仔煎</tiny-dropdown-item> | ||
| </tiny-dropdown-menu> | ||
| </template> | ||
| </tiny-dropdown> | ||
| <br /> | ||
| <br /> | ||
| <tiny-dropdown> | ||
| <template #dropdown> | ||
| <tiny-dropdown-menu :options="options" placement="bottom-start"> </tiny-dropdown-menu> | ||
| </template> | ||
| </tiny-dropdown> | ||
| </template> |
There was a problem hiding this comment.
Demo should demonstrate right-side expansion functionality
The template only shows bottom-start placement without demonstrating the new right-side expansion feature that this PR introduces. Consider adding examples that showcase both left and right expansion behaviors.
Suggested additions:
<tiny-dropdown>
<template #dropdown>
- <tiny-dropdown-menu placement="bottom-start">
+ <tiny-dropdown-menu placement="bottom-end">
<tiny-dropdown-item label="老友粉"></tiny-dropdown-item>
<!-- ... -->
</tiny-dropdown-menu>
</template>
</tiny-dropdown>
+<!-- Add documentation comment explaining the different placement options -->
+<tiny-dropdown>
+ <template #dropdown>
+ <tiny-dropdown-menu placement="bottom-start" expand="right">
+ <!-- Add example with right expansion -->
+ </tiny-dropdown-menu>
+ </template>
+</tiny-dropdown>Committable suggestion skipped: line range outside the PR's diff.
| // 展开图标的水平外边距 | ||
| --tv-DropdownItem-expand-margin-x: var(--tv-space-md, 8px); | ||
| // 自定义图标的水平外边距 | ||
| --tv-DropdownItem-pre-icon-margin-x: var(--tv-space-sm, 4px); |
There was a problem hiding this comment.
Add variables to support right-side expansion
While renaming margin variables to be more generic is good, additional variables are needed to fully support right-side expansion functionality.
Consider adding:
// 展开图标的水平外边距
--tv-DropdownItem-expand-margin-x: var(--tv-space-md, 8px);
// 自定义图标的水平外边距
--tv-DropdownItem-pre-icon-margin-x: var(--tv-space-sm, 4px);
+// 右侧展开时的位置调整
+--tv-DropdownItem-expand-right-offset: var(--tv-space-xl, 16px);
+// 右侧展开时的图标旋转
+--tv-DropdownItem-expand-right-rotation: 270deg;📝 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.
| // 展开图标的水平外边距 | |
| --tv-DropdownItem-expand-margin-x: var(--tv-space-md, 8px); | |
| // 自定义图标的水平外边距 | |
| --tv-DropdownItem-pre-icon-margin-x: var(--tv-space-sm, 4px); | |
| // 展开图标的水平外边距 | |
| --tv-DropdownItem-expand-margin-x: var(--tv-space-md, 8px); | |
| // 自定义图标的水平外边距 | |
| --tv-DropdownItem-pre-icon-margin-x: var(--tv-space-sm, 4px); | |
| // 右侧展开时的位置调整 | |
| --tv-DropdownItem-expand-right-offset: var(--tv-space-xl, 16px); | |
| // 右侧展开时的图标旋转 | |
| --tv-DropdownItem-expand-right-rotation: 270deg; |
| var(--tv-DropdownItem-icon-size) + var(--tv-DropdownItem-expand-margin-x) | ||
| ); | ||
| // 菜单项文本的左侧外边距 | ||
| --tv-DropdownItem-label-margin-left: calc( | ||
| var(--tv-DropdownItem-icon-size) + var(--tv-DropdownItem-pre-icon-margin-right) | ||
| var(--tv-DropdownItem-icon-size) + var(--tv-DropdownItem-pre-icon-margin-x) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update margin calculations for right-side expansion
The current margin calculations need to be updated to handle both left and right expansion scenarios.
Consider updating the calculations:
--tv-DropdownItem-content-margin-left: calc(
- var(--tv-DropdownItem-icon-size) + var(--tv-DropdownItem-expand-margin-x)
+ var(--tv-expand-direction) * (var(--tv-DropdownItem-icon-size) + var(--tv-DropdownItem-expand-margin-x))
);
--tv-DropdownItem-label-margin-left: calc(
- var(--tv-DropdownItem-icon-size) + var(--tv-DropdownItem-pre-icon-margin-x)
+ var(--tv-expand-direction) * (var(--tv-DropdownItem-icon-size) + var(--tv-DropdownItem-pre-icon-margin-x))
);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/dropdown-item/index.less (1)
159-203: Consider adding RTL support for right-side expansion.The right-side expansion implementation looks good, but consider adding RTL (Right-to-Left) support for better internationalization.
Add RTL support by using logical properties:
.@{dropdown-menu-prefix-cls}[x-placement='bottom-start'], .@{dropdown-menu-prefix-cls}[x-placement='top-start'] { .@{dropdown-item-prefix-cls}__wrap { - flex-direction: row-reverse; + flex-direction: row-reverse; /* For LTR */ + [dir="rtl"] & { + flex-direction: row; /* For RTL */ + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
examples/sites/demos/apis/dropdown.js(2 hunks)packages/theme/src/dropdown-item/index.less(4 hunks)
🔇 Additional comments (3)
packages/theme/src/dropdown-item/index.less (2)
17-17: LGTM: Variable declaration follows naming convention.
The new variable @dropdown-menu-prefix-cls follows the established pattern of using the css-prefix variable.
73-73: LGTM: Consistent margin variable naming.
The margin variables have been renamed from -right suffix to -x suffix, which better represents their bidirectional usage in the new right-side expansion feature.
Also applies to: 92-92
examples/sites/demos/apis/dropdown.js (1)
374-377: LGTM: Proper experimental feature marking.
The placement property is correctly marked as experimental with version 3.21.0.
| @@ -551,7 +554,7 @@ type IButtonType = 'default' | 'primary' | 'success' | 'warning' | 'danger' | 'i | |||
| name: 'IPlacementType', | |||
| type: 'type', | |||
| code: ` | |||
| type IPlacementType = 'top' | 'top-start' | 'top-end' | 'bottom' | 'bottom-start' | 'bottom-end' | |||
| type IPlacementType = 'bottom-start' | 'bottom-end' | |||
There was a problem hiding this comment.
💡 Codebase verification
Breaking Change Confirmed: IPlacementType restriction impacts existing code
The verification shows extensive usage of 'top' related placement values across the codebase, particularly in:
- Multiple tooltip components using 'top', 'top-start', and 'top-end' placements
- Popover components relying on top placements
- Various UI components (Select, Grid, Tree) using top positioning
The removal of these options from IPlacementType will break existing implementations. Specific impacts:
- 80+ instances of top-related placements found in component templates
- Test files containing top placement assertions
- Demo and documentation examples showcasing top placement functionality
Recommendations:
- Maintain these placement options in the type definition
- If removal is necessary, implement a deprecation cycle first
- Document migration path in release notes
🔗 Analysis chain
Breaking Change: IPlacementType restriction.
The removal of 'top', 'top-start', and 'top-end' options from IPlacementType is a breaking change that could affect existing implementations.
Let's verify the impact:
Consider:
- Adding a migration guide
- Deprecating the removed options first
- Maintaining backward compatibility until the next major version
Let me gather more information to verify the impact comprehensively.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of removed placement values
rg -l "placement.*['\"]top(?:-start|-end)?['\"]" --type vue
# Also check documentation
rg "placement.*top" examples/sites/demos/apis/dropdown.js
Length of output: 242
Script:
#!/bin/bash
# Search for usage of placement prop with top values in all files
rg -A 2 "placement['\"]?\s*[:=]\s*['\"]top"
# Search for IPlacementType usage
rg "IPlacementType"
# Search for any documentation or examples mentioning placement
fd "README|CHANGELOG|docs" --type f --exec rg -l "placement"
Length of output: 41346
MomoPoppy
force-pushed
the
feat-dropdown-1204
branch
from
abd9b06 to
64bdaaa
Compare
…down
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
Improvements
max-height,multi-stage, and refined type definitions.Documentation
placementattribute for dropdown positioning.