-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(dropdown-item): [dropdown] fix issues for v-auto-tip and add effect attr #2283
fix(dropdown-item): [dropdown] fix issues for v-auto-tip and add effect attr #2283
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/vue/src/dropdown-item/src/index.ts (1)
88-90
: Approve addition ofeffect
property, but documentation needed.The new
effect
property has been successfully added todropdownItemProps
. This appears to introduce a new customization option for dropdown items, which is a good enhancement.However, to ensure proper usage and maintainability:
- Please add a comment explaining the purpose of this property.
- Document the possible values it can take (e.g., 'light', 'dark', etc.).
- If applicable, update the component's documentation to reflect this new property.
Consider adding a comment like this above the property:
/** * The visual effect of the dropdown item. * Possible values: 'light' (default), 'dark', etc. */ effect: { type: String, default: 'light' }packages/vue/src/dropdown-item/src/pc.vue (2)
16-16
: Improved tooltip flexibility and customization.The changes to the
v-auto-tip
directive enhance the component's flexibility by conditionally applying the tooltip based on the presence ofgetTip
. The addition ofeffect
andplacement
properties also improves customization options.Consider extracting the tooltip configuration object into a computed property for better readability and maintainability. For example:
<script setup> // ... const tooltipConfig = computed(() => getTip ? { always: true, content: getTip, effect, placement: tipPosition } : false ) </script> <template> <li v-auto-tip="tooltipConfig" ...> <!-- ... --> </li> </template>This approach would make the template cleaner and allow for easier testing and modification of the tooltip configuration.
99-100
: New 'effect' prop added and improved readability.The addition of the 'effect' prop and the separation of props onto individual lines improve the component's customization options and code readability.
Consider enhancing the prop definitions by using the full object notation for better type checking and documentation:
props: { // ... other props tipPosition: { type: String, default: 'top' }, effect: { type: String, default: 'light', validator: (value) => ['light', 'dark'].includes(value) } }This approach provides better type checking, default values, and allows for value validation, improving the robustness of the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/vue/src/dropdown-item/src/index.ts (1 hunks)
- packages/vue/src/dropdown-item/src/pc.vue (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/vue/src/dropdown-item/src/index.ts (2)
87-87
: LGTM: Improved formatting fortipPosition
property.The
tipPosition
property has been moved to its own line, which improves the overall readability of thedropdownItemProps
object. This change is purely cosmetic and doesn't affect functionality.
87-90
: Overall assessment: Good improvements, minor documentation needed.The changes to this file are minimal and focused, improving both functionality and readability:
- The formatting change for
tipPosition
enhances code readability.- The addition of the
effect
property introduces a new customization option for dropdown items.These changes align well with the PR objectives of fixing issues and adding attributes to the dropdown component. The only recommendation is to add documentation for the new
effect
property to ensure clear understanding and proper usage.packages/vue/src/dropdown-item/src/pc.vue (1)
Line range hint
1-111
: Overall assessment: Improvements align with PR objectives.The changes made to the dropdown item component successfully address the issues related to
v-auto-tip
and add theeffect
attribute as intended. The modifications enhance the component's flexibility and customization options, which aligns well with the PR objectives.Key improvements:
- Conditional application of the tooltip based on
getTip
.- Addition of
effect
andplacement
properties for tooltip customization.- New
effect
prop for additional styling options.While the changes are generally good, consider implementing the suggested improvements:
- Extract the tooltip configuration into a computed property for better maintainability.
- Enhance prop definitions with full object notation for improved type checking and documentation.
These refinements will further improve the component's robustness and maintainability.
@@ -13,7 +13,7 @@ | |||
<template> | |||
<!-- TINY-TODO: tiny-dropdown-menu__item命名不规范,后续统一有个迭代去掉 --> | |||
<li | |||
v-auto-tip="{ always: true, content: getTip, effect: 'dark', placement: tipPosition }" | |||
v-auto-tip="getTip ? { always: true, content: getTip, effect, placement: tipPosition } : false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
升级下这个组件包的版本号,这样才可以用github-action去发包
…ct attr
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
effect
property for dropdown items, allowing for enhanced tooltip customization.getTip
, improving flexibility and user experience.@opentiny/vue-dropdown-item
package from3.18.0
to3.18.1
.