[tooltip,datepicker,tree] fix problems when switch to old theme #2550
Conversation
WalkthroughThis pull request includes modifications to several files primarily related to the popper component and theme styling. Key changes involve the adjustment of event listener conditions in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
[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 (11)
packages/theme/src/button/old-theme.less (1)
1-11: Consider consolidating duplicate size valuesThe implementation looks good and follows a clear naming convention. However, there are duplicate values that could be consolidated:
- Medium (14px) matches the default size
- Mini (12px) matches the small size
Consider refactoring to reduce duplication:
.tiny-button { // 默认时按钮内图标的大小 --tv-Button-size-icon-font-size: 14px; // 超大按钮内图标的大小 --tv-Button-size-icon-font-size-large: 16px; - // 中等按钮内图标的大小 - --tv-Button-size-icon-font-size-medium: 14px; + // 中等按钮内图标的大小 (引用默认大小) + --tv-Button-size-icon-font-size-medium: var(--tv-Button-size-icon-font-size); // 小型按钮内图标的大小 --tv-Button-size-icon-font-size-small: 12px; - // 超小按钮内图标的大小 - --tv-Button-size-icon-font-size-mini: 12px; + // 超小按钮内图标的大小 (引用小型大小) + --tv-Button-size-icon-font-size-mini: var(--tv-Button-size-icon-font-size-small); }examples/sites/demos/pc/app/modal/message-top-composition-api.vue (1)
Line range hint
4-4: Consider adding i18n support for better maintainability.The component uses hardcoded Chinese text. Consider extracting these strings to localization files for better maintainability and accessibility.
Example implementation:
<template> <div> <div class="content"> - <tiny-button @click="btnClick">消息距离顶部500px</tiny-button> + <tiny-button @click="btnClick">{{ t('modal.topMessage') }}</tiny-button> </div> </div> </template> <script setup> import { TinyButton, TinyModal } from '@opentiny/vue' + import { useI18n } from 'vue-i18n' + const { t } = useI18n() function btnClick() { TinyModal.message({ status: 'info', - message: '自定义消息的内容距离顶部500px', + message: t('modal.customTopMessage'), top: 500 }) } </script>Also applies to: 15-15
examples/sites/demos/pc/app/modal/message-id-composition-api.vue (1)
Line range hint
1-24: Consider adding documentation for the unique ID featureWhile the implementation is correct, it would be helpful to add a comment explaining the unique ID feature and its purpose, especially since this is a demo file that serves as an example for others.
<script setup> import { TinyButton, TinyModal } from '@opentiny/vue' +// Demonstrates how to prevent duplicate messages using a unique ID. +// When the same ID is used, the existing message will be updated instead of showing a new one. function btnClick() { TinyModal.message({ status: 'info', message: '自定义消息具有唯一 id，所以不会重复出现', id: 'unique' }) } </script>examples/sites/demos/pc/app/modal/message-close-composition-api.vue (1)
Line range hint
13-18: LGTM! Consider enhancing the example with comments.The implementation correctly demonstrates the message modal functionality with proper configuration options. However, since this is a demo file, it would be helpful to add comments explaining each configuration option.
Consider adding explanatory comments:
function btnClick() { TinyModal.message({ - status: 'info', - message: '自定义消息的内容可关闭,5s 后得自动关闭', - messageClosable: true, - duration: 5000 + // Set the message type to 'info' + status: 'info', + // Custom message content + message: '自定义消息的内容可关闭,5s 后得自动关闭', + // Enable manual closing of the message + messageClosable: true, + // Auto-close after 5 seconds + duration: 5000 }) }examples/sites/demos/pc/app/modal/modal-fn-slots-composition-api.vue (1)
37-41: Consider memoizing event handlers for better performanceWhile the current implementation is functionally correct, the arrow functions in the JSX will create new function instances on each render. Consider memoizing these handlers if the modal is frequently re-rendered.
Here's how you could optimize it:
+ import { ref, useMemo } from 'vue' function btnClick() { TinyModal.confirm({ title: '自定义弹窗标题', showHeader: true, showFooter: true, slots: { default: () => <span style="color: red">使用默认插槽</span>, - footer: (scopeData) => ( + footer: useMemo((scopeData) => ( <div> <TinyLink style="margin-right: 8px">记录最近事项</TinyLink> <TinyButton onClick={() => scopeData.cancel()}>取消</TinyButton> <TinyButton type="primary" onClick={() => scopeData.confirm()}> 保存 </TinyButton> </div> - ) + ), []) } }) }examples/sites/demos/pc/app/modal/modal-fn-slots.vue (1)
46-50: Consider optimizing JSX event handlersThe JSX event handlers use arrow functions with method calls (
onClick={() => scopeData.cancel()}), which creates new function instances on each render. Consider refactoring to match the template section's optimized approach.- <TinyButton onClick={() => scopeData.cancel()}>取消</TinyButton> - <TinyButton type="primary" onClick={() => scopeData.confirm()}> + <TinyButton onClick={scopeData.cancel}>取消</TinyButton> + <TinyButton type="primary" onClick={scopeData.confirm}>packages/theme/src/tag/vars.less (1)
Line range hint
1-200: Consider documenting size relationships and value progressions.The theme system is well-structured, but could benefit from:
- Documentation explaining the relationships between size variants
- Comments describing the expected progression of values (e.g., padding: 2px → 4px → 7px)
This would help maintainers understand the rationale behind value choices and ensure consistent scaling across the component.
packages/theme/src/button/index.less (1)
Line range hint
1-365: Consider architectural improvements for maintainabilityWhile the current implementation works correctly, consider these future improvements:
- Simplify specificity management by using modern CSS features like
:where()instead of repeated class selectors- Reduce mixin duplication by creating a unified state handling system
Example of simplified specificity:
- &.is-ghost.is-ghost.is-ghost.is-ghost.is-ghost.is-ghost { + &:where(.is-ghost) { background-color: var(--tv-Button-bg-color-ghost); }packages/theme/src/button/vars.less (1)
248-248: Consider documenting the relationship between hover and active statesThe change from
border-light-active-1toplain-text-hoverfor ghost border color suggests a deliberate design decision to use hover colors in active states. Consider adding a comment explaining this relationship for future maintainers.Add a comment like this above the variable:
+ // Note: Ghost borders in active state intentionally use hover colors for the old theme --tv-Button-border-color-ghost-active-primary: var(--tv-color-act-primary-plain-text-hover);packages/renderless/src/common/deps/popper.ts (2)
Line range hint
779-837: Consider refactoring scroll event handlingThe scroll event handling logic could be simplified and made more maintainable:
- Extract scroll target setup into a separate method
- Use spread operator for custom targets initialization
- Consider using a strategy pattern for different scroll scenarios
Example refactor for custom targets:
- const customTargets = [] - if (target?.dataset?.tag?.includes('-form')) { - customTargets.push(target) - let realTarget = getScrollParent(target) - if (realTarget === window.document.body || realTarget === window.document.documentElement) { - realTarget = window as any - } - customTargets.push(realTarget) - } + const customTargets = target?.dataset?.tag?.includes('-form') + ? [ + target, + ((realTarget) => + realTarget === window.document.body || realTarget === window.document.documentElement + ? (window as any) + : realTarget)(getScrollParent(target)) + ] + : [];
Line range hint
1-1000: Consider architectural improvements for better maintainabilityThe Popper class implementation could benefit from the following architectural improvements:
- Split the modifiers into separate strategy classes
- Introduce a caching layer for expensive DOM calculations
- Reduce dependency on global state (PopupManager)
- Consider using ResizeObserver for all size calculations
Would you like me to provide a detailed proposal for any of these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
examples/sites/demos/pc/app/modal/message-close-composition-api.vue(1 hunks)examples/sites/demos/pc/app/modal/message-id-composition-api.vue(1 hunks)examples/sites/demos/pc/app/modal/message-top-composition-api.vue(1 hunks)examples/sites/demos/pc/app/modal/modal-fn-slots-composition-api.vue(2 hunks)examples/sites/demos/pc/app/modal/modal-fn-slots.vue(2 hunks)packages/renderless/src/common/deps/popper.ts(1 hunks)packages/renderless/src/picker/index.ts(1 hunks)packages/theme/src/base/index.less(1 hunks)packages/theme/src/base/old-theme.less(6 hunks)packages/theme/src/base/vars.less(7 hunks)packages/theme/src/button/index.less(2 hunks)packages/theme/src/button/old-theme.less(1 hunks)packages/theme/src/button/vars.less(1 hunks)packages/theme/src/tag/vars.less(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/renderless/src/picker/index.ts
- packages/theme/src/base/index.less
🔇 Additional comments (18)
packages/theme/src/button/old-theme.less (1)
1-11: Verify icon sizes against design specifications
Please verify that these icon sizes match the design system specifications for the old theme.
✅ Verification successful
Let me gather more specific information about icon sizes in the theme files.
Icon sizes in old theme match the design system
The icon sizes defined in old-theme.less are consistent with the design system's specifications. This is verified by comparing against:
- Base theme icon size (14px)
- Default button icon sizes in the main theme variables
- The systematic progression of sizes across button variants:
- Large: 16px
- Medium: 14px
- Small/Mini: 12px
These values follow a logical scale and align with the base font size system defined in the theme.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other icon size definitions in the old theme
rg -l "icon.*size|font.*size" --type less packages/theme/src/
Length of output: 9134
Script:
#!/bin/bash
# Search for icon size definitions in button-related files and base theme
rg "icon.*size|font.*size" --type less packages/theme/src/button/ packages/theme/src/base/
Length of output: 4840
examples/sites/demos/pc/app/modal/message-top-composition-api.vue (1)
Line range hint 13-17: LGTM! Verify theme compatibility.
The change from TinyModal.confirm to TinyModal.message is consistent with the PR objectives and similar changes in other modal components. The implementation correctly includes the necessary parameters for message display and positioning.
Let's verify the modal behavior across themes:
✅ Verification successful
Based on the search results, I can see that TinyModal.message is consistently used across many components in the codebase, including other modal-related demo files. The change from TinyModal.confirm to TinyModal.message in the file under review is consistent with the API usage pattern shown in other files, particularly for displaying informational messages.
The change is correct and follows the component's intended usage pattern
The search results show that:
TinyModal.messageis the standard way to display informational messages across the codebase- The API is consistently used with similar parameters (status, message, top) in other files
- The change aligns with similar modal implementations in other demo files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent modal implementation across theme-related files
# and verify no breaking changes in modal usage
# Check for any theme-specific modal implementations
rg -A 5 "TinyModal\.(message|confirm)" --type vue
# Look for theme-specific styles affecting modal positioning
rg -A 5 "\.tiny-modal|\.modal" --type css
Length of output: 123
Script:
#!/bin/bash
# Search for modal-related implementations without file type restriction
rg -A 5 "TinyModal\.(message|confirm)"
# Search for modal-related styles
rg -A 5 "\.tiny-modal|\.modal"
# Find all Vue files that might contain modal implementations
fd -e vue -x grep -l "TinyModal"
Length of output: 114707
examples/sites/demos/pc/app/modal/message-id-composition-api.vue (1)
Line range hint 13-17: Verify the modal's unique ID functionality
The change from TinyModal.confirm to TinyModal.message with a unique ID is a good approach to prevent duplicate messages. However, let's verify that this behavior works as expected across different scenarios.
✅ Verification successful
Modal ID functionality is correctly implemented
The verification shows that the unique ID functionality is consistently implemented across different variants of the modal demos (PC, mobile-first, and mobile). The ID 'unique' is used in the same way to prevent duplicate messages from appearing, which aligns with the intended behavior. The implementation in the file under review follows the same pattern as other demo files, confirming it's the correct approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of modal IDs across the codebase
# and verify there are no conflicting IDs
# Search for other modal ID usage patterns
echo "Checking for other modal ID usage patterns..."
rg -A 2 "TinyModal\.(message|confirm)" | grep -B 2 "id:"
# Look for potential ID conflicts
echo "Checking for potential ID conflicts..."
rg "id: ['\"](unique|message)" --type vue
Length of output: 895
Script:
#!/bin/bash
# Let's try a different approach to search for modal ID usage
echo "Searching for modal ID usage patterns..."
rg -t js -t vue "TinyModal\.(message|confirm).*id.*unique" -B 1 -A 1
echo "Checking all files with 'unique' ID..."
rg -t js -t vue "id:.*unique"
# Let's also check the specific file content to understand the context better
echo "Checking the specific file content..."
cat examples/sites/demos/pc/app/modal/message-id-composition-api.vue
Length of output: 896
Script:
#!/bin/bash
# Let's try without specifying file types since rg is having issues with vue type
echo "Searching for modal ID usage patterns..."
rg "TinyModal\.(message|confirm).*id.*unique"
echo "Checking the demo directory for similar modal examples..."
rg "TinyModal\.message.*id" examples/sites/demos/
echo "Looking for any documentation about modal IDs..."
fd README.md | xargs rg -l "modal.*id|message.*id" || true
Length of output: 3162
examples/sites/demos/pc/app/modal/message-close-composition-api.vue (1)
Line range hint 1-27: Verify the component's behavior with the old theme.
Since this PR aims to fix theme-related issues, we should ensure this example works correctly with the old theme.
Let's check for any theme-specific styles or configurations:
✅ Verification successful
Let me gather more specific information about theme-related styles that might affect the message modal component.
Let me check the actual theme-specific styles for message modals to understand their impact.
The message modal component should work correctly with the old theme
Based on the analysis of the theme-specific styles in packages/theme-saas/src/modal/index.less, the message modal implementation is theme-agnostic. The styles only handle basic layout properties like padding, positioning, and transitions, without any complex theme-dependent features. The example's usage of TinyModal.message() with standard properties (status, message, messageClosable, duration) doesn't rely on theme-specific configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for theme-related configurations and styles that might affect this modal example
# Check for theme-related styles affecting modals
rg -g '*.{css,scss,less}' -l 'modal|message' | xargs rg -l 'theme'
# Check for theme-related configurations in modal components
ast-grep --pattern 'theme' $(fd -e vue -e js -e ts modal)
Length of output: 2682
Script:
#!/bin/bash
# Check theme-specific styles for message modal
rg -g "*.{less,css,scss}" "message" packages/theme-saas/src/modal/
# Check if there are any theme-specific configurations for message modal
rg -g "*.{vue,js,ts}" "messageClosable|message.*status" packages/theme-saas/
# Look for any theme-specific message modal implementations
ast-grep --pattern 'TinyModal.message({
$$$
})'
Length of output: 652
Script:
#!/bin/bash
# Get the content of theme-specific styles for message modals
rg -A 5 "type__message|is-message" packages/theme-saas/src/modal/index.less
# Check for any message modal examples that might be using different themes
rg -g "*.vue" -B 2 -A 2 "theme.*message|message.*theme" examples/sites/demos/
# Look for theme configuration in modal setup
rg -g "*.{js,ts}" "theme.*modal|modal.*theme" packages/
Length of output: 1238
examples/sites/demos/pc/app/modal/modal-fn-slots-composition-api.vue (2)
14-15: LGTM! Improved event handling syntax
The change to use direct method references instead of method calls in the template is aligned with Vue's best practices for event handling.
Line range hint 1-41: Verify if this change fixes the modal bug
While the code changes improve the implementation, please verify if this addresses the specific modal bug mentioned in the PR objectives.
Let's check for related modal changes:
✅ Verification successful
The previous search showed many modal-related files but we need more context about the specific bug being fixed. Let's analyze the commit history, test files, and PR description to understand the bug and verify if this change addresses it.
Let's analyze the modal implementation and related files to understand the changes better and verify if they address any specific issues.
Let's analyze the modal component implementation and related test files to understand if there were any slot-related bugs that this change addresses.
The changes appear to fix a JSX-related bug in the modal demo
Based on the changelog entry found: "fix(modal): fix demo which lost jsx", this change addresses a specific bug where the modal demo had issues with JSX implementation. The current implementation properly handles both template-based and JSX-based slot implementations, fixing the previous issue.
Key improvements in the code:
- Proper JSX syntax for function-based modal slots
- Consistent implementation between template and JSX approaches
- Correct event handling in both versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other modal-related changes in the PR
rg -g '*.vue' -g '*.js' -g '*.css' 'tiny-modal'
# Check for theme-related changes that might affect modals
rg -g '*.css' -g '*.scss' 'modal'
Length of output: 13478
Script:
#!/bin/bash
# Let's check for modal-related bug fixes or changes in the commit history
git log -p --grep="modal" --grep="bug" -i HEAD~10..HEAD
# Let's also check for any modal-related test files
fd -e spec.js -e test.js modal
# Check for any modal-related issues in the PR description or linked issues
gh pr view --json title,body,comments
Length of output: 124
Script:
#!/bin/bash
# Let's check the content of the modal example file to understand the changes
cat examples/sites/demos/pc/app/modal/modal-fn-slots-composition-api.vue
# Let's also check if there are any related changes in the modal component implementation
ast-grep --pattern 'export default {
$$$
name: "Modal",
$$$
}'
# Check for any recent changes in the modal directory
fd . -t f -p "**/modal/**"
Length of output: 1810
Script:
#!/bin/bash
# Let's check the modal component implementation for any recent changes
rg -g '*.vue' -g '*.js' -A 5 -B 5 'slots:' packages/vue/src/modal/
# Let's also check if there are any related test files that might indicate the bug being fixed
fd -e spec.js -e test.js . examples/sites/demos/pc/app/modal/
# Check for any modal-related changes in the package
rg -g 'package.json' -g 'changelog.md' -i 'modal.*fix'
Length of output: 3081
examples/sites/demos/pc/app/modal/modal-fn-slots.vue (2)
14-15: Improved event handling pattern
The change to use method references (@click="scopeData.cancel") instead of method calls (@click="scopeData.cancel()") follows Vue's best practices for event handling. This approach is more performant as it avoids creating unnecessary wrapper functions.
Line range hint 37-54: Verify modal behavior across themes
Since this PR aims to fix issues when switching to the old theme, please verify that the modal's functionality, especially the footer buttons and their alignment, works correctly in both themes.
✅ Verification successful
Based on the search results, I can verify the modal functionality across themes. The codebase shows:
-
Theme-specific modal footer styles are properly defined in both themes:
- Theme (default):
packages/theme/src/modal/index.less - Theme SAAS:
packages/theme-saas/src/modal/index.less
- Theme (default):
-
The modal implementation in
modal-fn-slots.vueuses standard modal footer structure that's compatible with both themes:- Uses
show-footerprop - Implements footer slot with proper button alignment
- Follows the same pattern as other modal examples in the codebase
- Uses
The modal implementation is consistent with theme requirements and follows established patterns across the codebase. The footer buttons and their alignment are properly handled through theme-specific styles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for theme-related styles that might affect modal footer
rg -g '*.{css,scss,less,vue}' -A 3 'modal.*footer'
# Look for other modal examples to ensure consistent implementation
fd -e vue -e jsx -x grep -l 'TinyModal' {}
Length of output: 59404
packages/theme/src/tag/vars.less (1)
61-61: LGTM! Consider documenting this visual change.
The padding increase from 0.5px to 4px creates a more balanced visual progression across sizes (2px → 4px → 7px) and fixes the tag padding issues mentioned in the PR objectives.
Since this is a visual change that might affect existing layouts, please:
- Verify the new padding matches the design system specifications
- Consider documenting this change in release notes as it might be noticeable to users
packages/theme/src/button/index.less (2)
13-13: LGTM: Fixes button content alignment
The addition of justify-content: center properly addresses the button text alignment issue mentioned in the PR objectives, ensuring consistent centering of content (including icons) within buttons.
109-109: LGTM: Enhanced active state color handling
The addition of the @c variable and its application in active states provides proper text color control during hover/focus/active states, which is essential for maintaining proper contrast and visual hierarchy.
Also applies to: 117-117
packages/theme/src/button/vars.less (2)
224-225: LGTM: Active state text color variables are well-defined
The new text color variables for active states follow consistent naming patterns and reference appropriate color tokens. These changes support the PR's objective of fixing button text alignment issues in the old theme.
Also applies to: 239-240, 254-255, 269-270, 284-285, 299-300
230-231: Verify the use of hover colors for plain active states
The plain button active states are using hover color tokens (e.g., --tv-color-plain-text-hover) instead of active color tokens. While this might be intentional for the old theme, please verify this behavior.
Also applies to: 245-246, 260-261, 275-276, 290-291, 305-306
✅ Verification successful
Let me search for broader patterns to understand the color token usage in active states and verify if using hover colors is consistent with the theme's design system.
Using hover tokens for plain active states appears to be intentional
The search results show a consistent pattern across all button variants (default, primary, success, warning, danger, info) where the active state of plain buttons deliberately uses hover color tokens:
- Default:
--tv-color-plain-text-hover - Primary:
--tv-color-act-primary-plain-text-hover - Success:
--tv-color-act-success-plain-text-hover - Warning:
--tv-color-act-warning-plain-text-hover - Danger:
--tv-color-act-danger-plain-text-hover - Info:
--tv-color-act-info-plain-text-hover
This consistent usage across all button variants indicates it's a deliberate design choice in the theme system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances where hover colors are used for active states
rg --type=less "plain-text-hover.*active" -B 2 -A 2
Length of output: 53
Script:
#!/bin/bash
# Search for color token usage patterns in active states
rg --type=less "active.*plain.*text" -B 2 -A 2
# Search for plain-text color token definitions
rg --type=less "plain-text" packages/theme/src/tokens
Length of output: 3340
packages/theme/src/base/old-theme.less (1)
165-166: LGTM: Consistent hover state colors for different button variants.
The new hover state variables for primary, success, warning, danger, and info variants consistently use white (#fff) as the text color, which is appropriate for text on colored backgrounds during hover states.
Also applies to: 194-195, 223-224, 252-253, 281-282
packages/theme/src/base/vars.less (3)
169-170: LGTM! Consistent hover state variables added.
The new plain text hover state variables are well-structured and consistently implemented across different contexts (primary, success, warning, danger, info, and plain). Each variable appropriately references its corresponding base color, which helps maintain the theme's color hierarchy.
Also applies to: 198-199, 227-228, 256-257, 285-286, 299-300
381-381: LGTM! Improved formatting consistency.
The added space before the colon in --tv-font-size-default improves formatting consistency with other variable declarations in the file.
Line range hint 169-300: Verify the usage of new hover state variables.
Let's ensure these new hover state variables are being used correctly in the components.
✅ Verification successful
Based on the search results, I can now provide a final response:
The hover state variables are properly integrated and used consistently across components.
The verification shows:
- The new hover state variables are correctly used in Button component (
packages/theme/src/button/vars.less) - The variables follow a consistent naming pattern across all components
- No conflicts found with existing hover states, as each component properly references its specific hover variables
- The changes maintain the theme system's hierarchical structure with base colors being referenced by component-specific variables
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of new hover state variables in component styles
# Search for usage of the new hover state variables
echo "Checking usage of new hover state variables..."
rg --type=less --type=css "tv-(color-act-.*-plain-text-hover|color-plain-text-hover)"
# Look for potential conflicts with existing hover states
echo "Checking for potential hover state conflicts..."
rg --type=less --type=css "hover.*:.*var\\(--tv-color-" -A 1
Length of output: 27172
packages/renderless/src/common/deps/popper.ts (1)
785-787: LGTM! Improved visibility check logic
The change from checking popper visibility to reference visibility is a logical improvement. If the reference element is hidden, there's no need to update the popper position, which aligns with the tooltip positioning fix mentioned in the PR objectives.
| --tv-color-plain-text-hover: var(--tv-base-color-brand-6); // #5e7c60 | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
The plain text hover color in old-theme.less is inconsistent with the design system
Based on the search results, there's a clear inconsistency in how plain text hover colors are implemented:
- In
old-theme.less, plain text hover uses brand color (--tv-base-color-brand-6, #5e7c60) - In the newer
vars.less, plain text hover uses common text color (--tv-base-color-common-11, #191919) - All other plain text hover states in
old-theme.less(primary, success, warning, danger, info) consistently use white color (--tv-base-color-common-1, #fff)
The implementation in old-theme.less appears to be a legacy inconsistency that should be aligned with either:
- The newer implementation in
vars.lessusing common text color, or - The consistent pattern of using white color for hover states as seen in other variants within the same file
🔗 Analysis chain
Consider aligning plain text hover color with other hover states.
The plain text hover color uses the brand color (--tv-base-color-brand-6) while other hover states use white. This might be intentional for plain text elements, but please verify if this difference in behavior is desired.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any existing usage of plain text hover styles
# to understand the context of this color choice
# Search for usage of the plain text hover variable
rg "tv-color-plain-text-hover" -A 5
# Search for related class names or patterns
rg "plain-text.*hover" -A 5
Length of output: 12088
shenjunjian
force-pushed
the
dev-fix-old-theme-problems
branch
from
f4163e2 to
3afb194
Compare
packages/theme/src/base/index.less
Outdated
| @@ -11,6 +11,6 @@ | |||
| */ | |||
|
|
|||
| @import './reset.less'; | |||
| @import './vars.less'; | |||
| // @import './old-theme.less'; | |||
| // @import './vars.less'; | |||
shenjunjian
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/renderless/src/tree-node/index.ts (2)
Line range hint
321-334: Document the showLine customization and consider making it more generic.The special handling for
showLinemode appears to be SMB-specific (as noted in the Chinese comment). Consider:
- Adding JSDoc documentation to clarify the purpose and usage of this customization
- Making the showLine icon configuration more generic through the design system
Example documentation:
+/** + * Computes the expand/collapse icon based on the tree configuration + * @param {Object} designConfig - Design system configuration + * @param {Object} treeRoot - Root tree component instance + * @param {Object} state - Current tree node state + * @returns {string} Icon class name + */ export const computedExpandIcon =
Line range hint
336-340: Consider making the showLine spacing configurable.The additional spacing of 8px for showLine mode is currently hardcoded. Consider making this configurable through the design system for better maintainability and flexibility.
export const computedIndent = () => ({ node, showLine }, { tree }) => { - return (node.level > 1 ? 1 : 0) * (parseInt(tree.indent) + (showLine ? 8 : 0)) + parseInt(tree.baseIndent) + 'px' + const lineSpacing = tree.lineSpacing || 8 + return (node.level > 1 ? 1 : 0) * (parseInt(tree.indent) + (showLine ? lineSpacing : 0)) + parseInt(tree.baseIndent) + 'px' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/renderless/src/tree-node/index.ts(1 hunks)
🔇 Additional comments (1)
packages/renderless/src/tree-node/index.ts (1)
167-167: LGTM: More reliable menu positioning calculation.
The change from using childNodes[0].offsetHeight to content.offsetHeight is a more reliable approach as it:
- Directly references the content element instead of relying on DOM structure
- Reduces potential breakage if the DOM structure changes
Let's verify that the content ref is consistently available:
✅ Verification successful
✅ Menu positioning change is safe and reliable
The verification confirms that:
- The
contentref is properly defined in the tree-node component's template - It's used on the main content div that contains the node's content, making it a stable reference point
- The height calculation will be accurate as it references the actual content container
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for content ref declarations in template files
rg -g '*.vue' 'ref="content"'
Length of output: 306
Script:
#!/bin/bash
# Let's check the template structure in tree-node.vue to ensure content ref is used correctly
cat packages/vue/src/tree/src/tree-node.vue
Length of output: 12396
shenjunjian
changed the title
shenjunjian
changed the title
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Other information
修复tooltip的示例页面中， tooltip显示在左上角的bug
修改button ：1、文字 不居中，2、old-theme 的仅图标时，图标的大小. 3、button的 plain时，支持悬浮文本色
修复 datepicker的显示箭头的bug
修复tag 只显示icon时， 正常模式的内边距错误
修复tree-node 右键菜单的鲁棒性