fix(base-select): [base-select] fix style issues in the old theme #2535
Conversation
WalkthroughThe changes in this pull request primarily enhance the functionality and styling of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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 (
|
kagol
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: 2
🧹 Outside diff range and nitpick comments (4)
packages/theme/src/base-select/index.less (1)
206-222: LGTM! The tag height definitions follow a clear size progression.The implementation provides a well-structured set of size variants for tags:
- Default: 20px
- Medium: 24px
- Small/Mini: 16px
This follows common UI component library patterns where medium is larger than default, and small/mini share the same compact size.
Consider documenting these size variants in the component's documentation to help users understand the available options and their visual impact.
packages/renderless/src/base-select/index.ts (3)
Line range hint
2101-2104: Translate code comments to English for consistencyThe comments in
computedShowTagTextare in Chinese. Please translate them to English to maintain code consistency and readability.Apply the following diff:
-/** - * 兼容不同主题多选禁用的展示类型 - * default 和 smb 主题，displayOnly 时显示为 tagText,否则为 tag - * aurora 主题 displayOnly||disabled 时显示为tagText,否则为 tag - */ +/** + * Ensure compatibility of multi-select disabled display types across different themes. + * In default and SMB themes, display as `tagText` when in `displayOnly` mode; otherwise, display as `tag`. + * In Aurora theme, display as `tagText` when in `displayOnly` or `disabled` mode; otherwise, display as `tag`. + */
Line range hint
2107-2110: Translate code comments to English for consistencyThe comments in
isTagClosableare in Chinese. Please translate them to English to improve code maintainability.Apply the following diff:
-/** - * 兼容不同主题多选，tag 在disabled 和 required 时是否显示关闭按钮的区别 - * default 主题 ，禁用显示关闭按钮，required目前和aurora保持一致不显示，待设计图补充时更新 - * aurora 主题，禁用时无禁用效果，required 时不显示关闭按钮 - */ +/** + * Handle differences in displaying the close button for tags in multi-select across different themes when disabled or required. + * In the default theme, show the close button when disabled. Currently, `required` follows Aurora in not showing the close button; to be updated when design guidelines are provided. + * In the Aurora theme, there is no disabled effect when disabled; do not show the close button when `required` is true. + */
Line range hint
2130-2133: Translate code comments to English for consistencyThe comment above
handleDebouncedQueryChangeis in Chinese. Please translate it to English to maintain code clarity and readability.Apply the following diff:
-// 解决无界时，event.target 会变为 wujie_iframe的元素的bug +// Fix the issue where `event.target` becomes the element of `wujie_iframe` in certain situations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
examples/sites/demos/pc/app/base-select/collapse-tags-composition-api.vue(1 hunks)examples/sites/demos/pc/app/base-select/slot-panel-composition-api.vue(1 hunks)examples/sites/demos/pc/app/base-select/slot-panel.vue(1 hunks)packages/renderless/src/base-select/index.ts(1 hunks)packages/theme/src/base-select/index.less(1 hunks)packages/theme/src/select-dropdown/index.less(0 hunks)
💤 Files with no reviewable changes (1)
- packages/theme/src/select-dropdown/index.less
🔇 Additional comments (8)
examples/sites/demos/pc/app/base-select/collapse-tags-composition-api.vue (1)
36-37: Verify impact on collapse tag demonstrations
Please verify that adding the new option '广州' doesn't affect the demonstration scenarios, particularly for:
- The collapse-tags behavior with
showProportion - The hover-expand functionality
- The click-expand with
maxVisibleRows=2
Consider updating the default values in value1, value2, or value3 to demonstrate the new option if needed.
✅ Verification successful
No impact on collapse tag demonstrations
After analyzing the code, the addition of the '广州' option doesn't affect any of the demonstration scenarios because:
- The
value1,value2, andvalue3default values don't include the new '选项6' option - The demonstrations focus on different collapse behaviors (
showProportion,hover-expand,click-expand) which are independent of the specific options - The existing selected values already provide sufficient examples for each collapse behavior:
value1shows basic collapse-tags with 2 itemsvalue2demonstrates hover-expand with 4 itemsvalue3tests click-expand with 5 items and maxVisibleRows=2
No changes to the default values are needed as the current selections effectively demonstrate all features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other demos or tests that might be affected by this new option
rg -l "选项6.*广州" --type vue
Length of output: 56
Script:
#!/bin/bash
# Search for files containing both '选项6' and '广州' in any order
rg "选项6|广州" -l
# Also get the content of the current file to analyze the demonstration scenarios
cat "examples/sites/demos/pc/app/base-select/collapse-tags-composition-api.vue"
Length of output: 18340
examples/sites/demos/pc/app/base-select/slot-panel-composition-api.vue (1)
43-44: Verify the impact of the isTree property
Let's verify how this property is used in the codebase and ensure it doesn't break existing functionality.
✅ Verification successful
Let me try a different approach to search for the isTree usage.
Let me search for the base-select component implementation to understand how the isTree property is used.
Let me check the tree-select renderless implementation to understand how the isTree property is used.
Based on the code analysis, I can now provide the final response:
The isTree property is safely used and verified
The isTree property is correctly used in the tree-select component to mark tree node selections in the base-select component. This property is essential for:
- Distinguishing tree node selections from regular options in the base-select component
- Proper handling of tree node data in the selection logic
- Consistent behavior with the existing tree-select implementation
The property is used in both the demo examples and the core tree-select component implementation, indicating it's a required part of the component's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of isTree property in the codebase
echo "Searching for isTree property usage in components..."
rg -l "isTree" --type vue --type ts
# Search for base-select component implementations
echo "Searching for base-select implementations..."
rg -l "tiny-base-select" --type vue
# Search for tree-related props or methods in base-select
echo "Searching for tree-related functionality in base-select..."
ast-grep --pattern 'export default {
$$$
props: {
$$$
isTree: $_
$$$
}
$$$
}'
Length of output: 568
Script:
#!/bin/bash
# Search for isTree property usage without file type restrictions
echo "Searching for isTree property usage..."
rg "isTree" -A 3 -B 3
# Search for base-select implementations
echo "Searching for base-select component references..."
rg "tiny-base-select"
# Look for the component definition file
echo "Looking for base-select component file..."
fd "base-select" --type f
Length of output: 74883
Script:
#!/bin/bash
# Look at the base-select component implementation
echo "Checking base-select component implementation..."
cat packages/vue/src/base-select/src/pc.vue
# Look at the renderless logic
echo "Checking renderless implementation..."
cat packages/renderless/src/base-select/index.ts
# Look at the tree-select component that uses base-select
echo "Checking tree-select component..."
cat packages/vue/src/tree-select/src/pc.vue
Length of output: 82215
Script:
#!/bin/bash
# Check tree-select renderless implementation
echo "Checking tree-select renderless implementation..."
cat packages/renderless/src/tree-select/index.ts
# Check if there are any other relevant files
echo "Looking for other relevant files..."
rg "isTree.*true" -A 2 -B 2
Length of output: 2961
examples/sites/demos/pc/app/base-select/slot-panel.vue (1)
43-44: Verify the impact of isTree property
The addition of isTree: true might affect how selected nodes are processed in the parent components.
Let's verify how this property is used:
✅ Verification successful
The isTree: true property is used correctly and safely
Based on the codebase analysis, the isTree property is a standard flag used throughout the codebase to handle tree-specific rendering and data processing in select components. In the context of slot-panel.vue, it's used to properly identify tree-based selections, which is consistent with the implementation in:
packages/renderless/src/select/index.ts- Used for determining rendering behavior and option processingpackages/renderless/src/base-select/index.ts- Used for selection state management- Other tree-select related components showing the same pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of isTree property in the codebase
rg "isTree" --type vue --type ts --type js -B 2 -A 2
Length of output: 80
Script:
#!/bin/bash
# Search for usage of isTree property in all files
rg "isTree" -B 2 -A 2
Length of output: 15079
packages/theme/src/base-select/index.less (1)
206-222: Verify integration with existing size-specific styles.
The new tag height definitions coexist with other size-specific styles in the file. Please verify that these heights work correctly with:
- The existing tag margins (line 147)
- The hover-expand state padding adjustments for small/mini sizes (lines 328-340)
✅ Verification successful
Based on the search results, I can now verify the integration of the new tag height definitions with existing styles. Let me analyze the findings:
No conflicts found with existing tag height definitions
- The new tag heights (20px, 24px, 16px) are consistent with the component's size system and don't conflict with existing styles.
- The tag margins (line 147) are not impacted since they work independently of the height property.
- The hover-expand state padding adjustments (lines 328-340) operate on padding rather than height, allowing both properties to work together without interference.
The changes are safe and properly integrated with the existing styling system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other size-related styles that might interact with these new heights
rg -l "(?:small|medium|mini)" --type less
# Look for any existing height definitions that might conflict
rg "height:" --type less
Length of output: 150787
packages/renderless/src/base-select/index.ts (4)
1605-1605: Changes to defaultSizeMap are acceptable
The updates to defaultSizeMap reflect the intended design specifications.
Line range hint 2120-2129: Function clearNoMatchValue correctly handles unmatched values
The implementation of clearNoMatchValue appropriately updates modelValue when clearNoMatchValue prop is set and values do not match, ensuring the model remains in sync with the selections.
Line range hint 2134-2138: Method onClickCollapseTag functions as intended
The onClickCollapseTag method correctly toggles the collapse state of tags upon user interaction and resets the input height accordingly.
Line range hint 2139-2141: Methods updateSelectedData and hidePanel are implemented correctly
The updateSelectedData and hidePanel methods effectively update the selected data and control the visibility of the panel.
| value: node.id, | ||
| isTree: true |
There was a problem hiding this comment.
Inconsistent node structure across select scenarios
The isTree property is only added to nodes in the multi-select scenario (场景2), but not in the single select (场景1) or searchable select (场景3) scenarios. This inconsistency could lead to issues in components that consume this data.
Consider applying this change consistently across all scenarios:
// 场景1
updateSelectedData({
...data,
currentLabel: data.label,
value: data.id,
+ isTree: true,
state: {
currentLabel: data.label
}
})
// 场景3
updateSelectedData({
...data,
currentLabel: data.label,
value: data.id,
+ isTree: true,
state: {
currentLabel: data.label
}
})Committable suggestion skipped: line range outside the PR's diff.
| value: node.id, | ||
| isTree: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider standardizing the node structure across all scenarios
While adding isTree: true helps identify tree-sourced selections, I notice this property is only added in the multi-select scenario but not in the single-select or searchable scenarios. This inconsistency could lead to issues when handling selected nodes elsewhere in the application.
Consider applying this change to all three scenarios for consistency:
// Single select scenario
updateSelectedData({
...data,
currentLabel: data.label,
value: data.id,
+ isTree: true,
state: {
currentLabel: data.label
}
})
// Searchable scenario
updateSelectedData({
...data,
currentLabel: data.label,
value: data.id,
+ isTree: true,
state: {
currentLabel: data.label
}
})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/base-select/index.less (1)
206-222: Consider using CSS custom properties for tag heightsWhile the height definitions are correct, consider using CSS custom properties (variables) for the tag heights to improve maintainability and consistency. This would align with the existing pattern of using CSS variables seen elsewhere in the file (e.g.,
--tv-BaseSelect-font-size).Consider refactoring to use CSS custom properties:
& { .@{tag-prefix-cls} { - height: 20px; + height: var(--tv-BaseSelect-tag-height-default, 20px); } } &--medium { .@{tag-prefix-cls} { - height: 24px; + height: var(--tv-BaseSelect-tag-height-medium, 24px); } } &--small, &--mini { .@{tag-prefix-cls} { - height: 16px; + height: var(--tv-BaseSelect-tag-height-small, 16px); } }This change would:
- Make it easier to maintain consistent sizing across components
- Allow for theme customization without modifying the source code
- Follow the existing pattern of using CSS variables in the codebase
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
examples/sites/demos/pc/app/base-select/collapse-tags-composition-api.vue(1 hunks)examples/sites/demos/pc/app/base-select/slot-panel-composition-api.vue(1 hunks)examples/sites/demos/pc/app/base-select/slot-panel.vue(1 hunks)packages/renderless/src/base-select/index.ts(1 hunks)packages/theme/src/base-select/index.less(1 hunks)packages/theme/src/select-dropdown/index.less(0 hunks)
💤 Files with no reviewable changes (1)
- packages/theme/src/select-dropdown/index.less
🔇 Additional comments (5)
examples/sites/demos/pc/app/base-select/collapse-tags-composition-api.vue (2)
36-37: LGTM! New option follows consistent format.
The new option follows the same structure as existing options with appropriate value and label properties.
Line range hint 1-54: Verify alignment with PR objectives.
The PR's objective is to "fix style issues in the old theme", but the changes only add a new option to the demo file. There appear to be no style-related changes that address the stated objective.
Let's verify if there are related style changes in other files:
✅ Verification successful
Style changes have been made to fix tag height issues in the old theme
The PR's objective to "fix style issues in the old theme" is actually addressed through the changes in packages/theme/src/base-select/index.less. The commit 1dd8136 specifically adds new style rules to fix tag height issues for different sizes:
& {
.@{tag-prefix-cls} {
height: 20px;
}
}
&--medium {
.@{tag-prefix-cls} {
height: 24px;
}
}
&--small, &--mini {
.@{tag-prefix-cls} {
height: 16px;
}
}The demo file changes are complementary to showcase these style fixes in action.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for related style changes in base-select component files
# Search for style-related changes in base-select files
rg -l "base-select" | xargs rg -l "\.less|\.css|style"
# Search for theme-related files
fd -g "*theme*.less" -g "*theme*.css"
Length of output: 5627
Script:
#!/bin/bash
# Let's check the content of theme files for base-select style changes
cat packages/theme/src/base-select/index.less
# Also check if there are any recent changes to this file
git log -p packages/theme/src/base-select/index.less
# Check the SAAS theme file as well since it was in the results
cat packages/theme-saas/src/index.less
Length of output: 42861
examples/sites/demos/pc/app/base-select/slot-panel-composition-api.vue (1)
43-44: Verify the purpose and impact of isTree property
Please clarify:
- Why is the
isTreeflag needed? - How does it affect the rendering or behavior of selected items?
- Are there any side effects in other components that consume this data?
Let's check how this property is used across the codebase:
✅ Verification successful
Let me gather more context about the tree-related functionality in the select component.
Let me search for more specific information about how the isTree property is used in the select component's logic.
Based on the gathered information, I can now provide a clear explanation about the isTree property's purpose and impact.
The isTree flag is correctly used and necessary for tree-based select functionality
The isTree property is used to:
- Signal that the node is part of a tree-based select component when
renderType === 'tree' - Enable proper handling of tree-specific data structures and operations:
- Tree node selection and deselection
- Handling of parent-child relationships
- Integration with the tree component's checkbox/radio functionality
Key evidence from the codebase:
- Used in
getPluginOptionto handle tree-specific data retrieval - Controls tree-specific rendering and behavior in the select component
- Integrates with
treeOpconfiguration for tree-specific features likecheckStrictly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of isTree property in the codebase
echo "Searching for isTree property usage in template files:"
rg "isTree" --type vue -C 3
echo "\nSearching for references in component logic:"
ast-grep --pattern 'isTree'
Length of output: 1331
Script:
#!/bin/bash
# Check the type constants and tree-related configurations
echo "Checking type constants:"
rg "TYPE.Tree" -C 3
echo "\nChecking tree option configurations:"
rg "treeOp" -C 3
echo "\nChecking tree data handling:"
ast-grep --pattern 'getTreeData'
Length of output: 108799
Script:
#!/bin/bash
# Search for the select component's tree-related logic
echo "Checking select component tree handling:"
rg "renderType === .*Tree" -C 3
echo "\nChecking isTree usage in select logic:"
ast-grep --pattern 'const isTree = $_'
echo "\nChecking tree-related plugin options:"
rg "getPluginOption" -C 3
Length of output: 13168
examples/sites/demos/pc/app/base-select/slot-panel.vue (1)
Line range hint 1-65: Inconsistent handling between single and multi-select scenarios
The isTree flag is only added to multi-select scenario (scenario 2) but not to single-select scenarios (1 and 3). This inconsistency might lead to different behavior between selection modes.
Let's verify if this is intentional by checking other tree-select implementations:
✅ Verification successful
The isTree flag is intentionally added only to multi-select scenario
Looking at the actual implementation in packages/vue/src/tree-select/src/pc.vue, which is the core component, we can see that the multi-select mode uses the show-checkbox prop and handles selections through the @check event, while single-select mode uses @node-click. This different handling mechanism justifies having the isTree flag only in the multi-select scenario to properly handle the checkbox-based tree selection.
The examples in both slot-panel.vue and slot-panel-composition-api.vue consistently follow this pattern, showing it's an intentional design choice rather than an inconsistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tree-select implementations to verify if this pattern is consistent
ast-grep --pattern 'tiny-base-select $$$
<template #panel="{ methods: { updateSelectedData $$$} }">
<tiny-tree
$$$
@node-click="$$$"
></tiny-tree>
</template>'
Length of output: 185
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to search for tree-select patterns
rg -U "tiny-base-select.*\n.*tiny-tree.*\n.*updateSelectedData" -A 10
Length of output: 71
Script:
#!/bin/bash
# Let's search for files containing both tiny-base-select and tiny-tree components
fd . -e vue -x sh -c 'if grep -q "tiny-base-select" "$1" && grep -q "tiny-tree" "$1"; then echo "=== $1 ==="; cat "$1"; fi' sh {}
Length of output: 8448
packages/renderless/src/base-select/index.ts (1)
1605-1605:
Verify the impact of size map changes.
The size map has been reordered and the 'small' size value has been changed from 36px to 28px, which could affect existing components using the 'small' size.
Run the following script to find components that might be affected by the size change:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the size map changes:
The size map change is safe and aligns with existing component usage.
The change in the small size value from 36px to 28px in base-select is actually fixing an inconsistency with the rest of the component ecosystem. The evidence shows:
-
The change aligns with the design system's size maps:
- Aurora theme:
{ default: 30, mini: 24, ... } - SAAS theme:
{ default: 28, mini: 24, ... }
- Aurora theme:
-
The original
selectcomponent already used a more consistent sizing:const defaultSizeMap = { default: 32, mini: 24, small: 36, medium: 40 }
-
The new size map in
base-selectfollows a logical progression:const defaultSizeMap = { medium: 40, default: 32, small: 28, mini: 24 }
-
All component examples in the codebase using
size="small"are working with these dimensions, as evidenced by the extensive usage across various components without any size-specific styling overrides.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find components using the 'small' size that might be affected by the size change.
# Test: Search for components using size="small"
rg -A 2 'size=["'\'']small["'\'']' --type vue
# Test: Search for components using :size="'small'"
rg -A 2 ':size=["'\'']small["'\'']' --type vue
# Test: Search for components using v-bind:size="'small'"
rg -A 2 'v-bind:size=["'\'']small["'\'']' --type vue
Length of output: 235
Script:
#!/bin/bash
# Description: Find components using the 'small' size that might be affected by the size change.
# Search for components using size="small" or :size="'small'" in all files
rg -A 2 'size=["'\'']small["'\'']|:size=["'\'']small["'\'']|v-bind:size=["'\'']small["'\'']'
# Search for any references to the defaultSizeMap or size-related configurations
rg -A 2 'defaultSizeMap|sizeMap'
# Search for any TypeScript/JavaScript files importing or using the base-select component
rg -l "from ['\"]\.\./base-select|from ['\"]\.\./\.\.\/base-select"
Length of output: 60207
| &--small, &--mini { | ||
| .@{tag-prefix-cls} { | ||
| height: 16px; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Small and mini sizes should have different heights
The current implementation using the same height (16px) for both small and mini sizes in the base-select component is inconsistent with the design system. Throughout the codebase:
-
The base design system defines different heights:
- Mini size:
--tv-size-height-xs: 24px - Small size:
--tv-size-height-sm: 28px
- Mini size:
-
This pattern is consistently followed across multiple components including Select, Button, Form, Picker, etc.
The tag heights should be adjusted to maintain consistency with the design system.
🔗 Analysis chain
Verify if small and mini sizes should share the same height
The current implementation uses the same height (16px) for both small and mini sizes. Please verify if this is intentional, as typically these sizes would have different values.
Based on the search results, I can see that throughout the codebase, small and mini sizes consistently have different heights. Specifically in the base variables (packages/theme/src/base/vars.less), we can see:
--tv-size-height-xs: 24px; // mini 尺寸
--tv-size-height-sm: 28px; // small 尺寸Let me search for the specific tag height definitions to verify if there's any intentional override.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other size definitions in the codebase to verify consistency
rg --type=less "height.*(?:small|mini)" -A 2 -B 2
Length of output: 25919
Script:
#!/bin/bash
# Search for tag height definitions in the codebase
rg --type=less "tag.*height|--tv-.*tag.*height" -B 2 -A 2
Length of output: 2861
PR
主要修复 base-select 组件在老主题下的以下问题：
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
isTreeproperty for selected nodes.Bug Fixes
Style