fix(dropdown): [dropdown] rename getTip api in dropdown-item; replace reset.less cssvars by --tv- vars #2331
Conversation
WalkthroughThe changes in this pull request primarily involve the refactoring of the tooltip handling in the dropdown item component. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
shenjunjian
changed the title
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/theme/src/scrollbar/index.less (1)
87-133: LGTM! Changes align well with PR objectives.The new media query and its contents effectively implement the scrollbar styles using the
--tv-prefixed CSS variables, which aligns with the PR objective of replacing reset.less cssvars. The comprehensive styling for both default and minimum scrollbar classes enhances the component's flexibility.Consider using CSS variables for all border-radius values.
For consistency and easier maintenance, consider using CSS variables for all border-radius values, including the one on line 123.
Here's a suggested change:
- border-radius: 2px; + border-radius: var(--tv-border-radius-scrollbar-thumb-min);Don't forget to define this new variable in your variables file.
Add a comment explaining the media query's purpose.
To improve code readability and maintainability, consider adding a brief comment explaining the purpose of this media query.
Here's a suggested addition:
+// Apply custom scrollbar styles only on larger screens (tablets and above) @media (min-width: 768px) { // ... (existing code) }packages/vue/src/dropdown-item/src/pc.vue (2)
16-18: LGTM! Consider destructuringstatefor cleaner template syntax.The update to use
state.computedTipaligns well with Vue's reactivity model and the refactoring mentioned in the summary. The addition ofeffectandtipPositionenhances the tooltip's customization options.Consider destructuring
statein thesetupfunction to allow for a cleaner template syntax:-v-auto-tip=" - state.computedTip ? { always: true, content: state.computedTip, effect, placement: tipPosition } : false -" +v-auto-tip=" + computedTip ? { always: true, content: computedTip, effect, placement: tipPosition } : false +"This change would require updating the
setupfunction to return the destructuredcomputedTip.
Line range hint
91-94: LGTM! Consider adding prop validation and documentation.The addition of these new props enhances the component's flexibility and aligns with the changes seen in the template.
To improve maintainability and developer experience, consider:
- Adding prop validation for the new props. For example:
appendToBody: { type: Boolean, default: false }, textField: { type: String, default: 'text' }, tip: { type: [String, Function], default: '' }, tipPosition: { type: String, default: 'top' }
Adding JSDoc comments to describe the purpose and expected values for each new prop.
Updating the component's documentation to reflect these new props and their usage.
packages/renderless/src/dropdown-item/index.ts (1)
189-197: Approve changes with a minor suggestion for clarity.The refactoring of
getTiptocomputedTipis a good improvement. It aligns with Vue's reactivity model and provides more flexibility in computing the tip. The function correctly handles both function and non-functiontipprops, maintaining backwards compatibility.Consider adding a type annotation for the return value of the inner function for improved clarity:
export const computedTip = ({ props, vm }: Pick<IDropdownItemRenderlessParams, 'props' | 'vm'>): () => string => () => { if (props.tip && typeof props.tip === 'function') { return props.tip({ itemData: props.itemData, vm }) } return props.tip || '' }This change explicitly declares that the inner function returns a string, which matches the expected behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/renderless/src/dropdown-item/index.ts (1 hunks)
- packages/renderless/src/dropdown-item/vue.ts (4 hunks)
- packages/theme/src/base/reset.less (2 hunks)
- packages/theme/src/base/vars.less (1 hunks)
- packages/theme/src/scrollbar/index.less (1 hunks)
- packages/vue/src/dropdown-item/src/pc.vue (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
packages/vue/src/dropdown-item/src/pc.vue (1)
Line range hint
1-114: Overall, the changes look good and align with the PR objectives.The modifications to the
DropdownItemcomponent improve its reactivity and add new customization options. The shift fromgetTiptocomputedTipenhances consistency with Vue's reactivity model, while the new props provide more flexibility in usage.A few minor suggestions have been made to further improve code quality and developer experience:
- Consider destructuring
statein thesetupfunction for cleaner template syntax.- Add prop validation for the new props.
- Enhance documentation with JSDoc comments and update the component's documentation to reflect the new props and their usage.
These changes contribute positively to the component's functionality and maintainability.
packages/renderless/src/dropdown-item/vue.ts (6)
40-40: LGTM: Import statement updated correctly.The addition of
computedTipto the import statement is consistent with the renaming ofgetTiptocomputedTipas mentioned in the PR objectives.
58-58: LGTM: API array updated correctly.The replacement of
'getTip'with'computedTip'in theapiarray is consistent with the renaming of the function and ensures that the newcomputedTipfunction is properly exposed in the API.
83-83: Clarify the purpose of addingchildren: [].The addition of
children: []to the state object seems unrelated to the PR objectives. Could you please explain the purpose of this change? If it's not necessary, consider removing it to keep the changes focused on the stated objectives.
84-84: LGTM:computedTipadded as a computed property.The addition of
computedTipas a computed property in the state object is consistent with the renaming ofgetTiptocomputedTipand aligns with the PR objectives.
111-111: LGTM: API initialization updated correctly.The replacement of
getTip: getTip({ props, vm })withcomputedTip: computedTip({ props, vm })in theinitApifunction is consistent with the renaming of the function and ensures that the newcomputedTipfunction is properly initialized in the API.
Line range hint
1-148: Overall, the changes align with the PR objectives, but one clarification is needed.The renaming of
getTiptocomputedTiphas been consistently implemented throughout the file, including updates to imports, API array, state initialization, and API initialization. These changes align well with the PR objectives and the AI-generated summary.However, the addition of
children: []to the state object (line 83) seems unrelated to the stated objectives. Please provide clarification on the purpose of this addition or consider removing it if it's not necessary for this PR.packages/theme/src/base/vars.less (1)
441-441: Approved: Scrollbar height adjustment improves UI consistencyThe change from 80px to 8px for the scrollbar height is a good improvement. This adjustment:
- Aligns the scrollbar height with the scrollbar width (8px).
- Provides a more standard and less obtrusive scrollbar size.
- Likely enhances the overall aesthetics and usability of scrollable elements in the UI.
This change should result in a more consistent and polished user interface across the application.
| @@ -52,7 +31,7 @@ | |||
|
|
|||
| // 高亮节点 | |||
| .tiny-hl-query-node { | |||
| color: var(--ti-common-color-text-highlight) !important; | |||
| color: var(----tv-color-text-active) !important; | |||
There was a problem hiding this comment.
Fix the CSS variable name to ensure proper styling.
The CSS variable var(----tv-color-text-active) appears to have an extra pair of hyphens. CSS custom properties should begin with two hyphens --. Please correct it to var(--tv-color-text-active).
Apply this diff to fix the variable name:
-color: var(----tv-color-text-active) !important;
+color: var(--tv-color-text-active) !important;📝 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.
| color: var(----tv-color-text-active) !important; | |
| color: var(--tv-color-text-active) !important; |
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
修改dropdown-item中的getTip函数
滚动条的cssvar替换
scrollbar的样式移到 scrollbar组件的最外层了
Summary by CodeRabbit
Release Notes
New Features
appendToBody,textField,tip, andtipPosition.Bug Fixes
Style