refactor(base-select): [base-select] refactor vars for base-select #2399
Conversation
WalkthroughThe changes introduce new LESS styles and variables for the base select component, enhancing its design and functionality. A new stylesheet Changes
Possibly related PRs
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 (
|
MomoPoppy
force-pushed
the
MomoPoppy/refactor-base-select-1025
branch
from
6051890 to
09447a2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
packages/theme/src/base-select/vars.less (1)
13-19: Consider adding English comments for better international collaboration.The variable definitions are well-structured with appropriate fallbacks, but the comments are in Chinese. Consider adding English translations or replacing with English comments to improve maintainability for international contributors.
Example:
- // 文本色 + // Text color - // 字号 + // Font size - // 行高 + // Line heightpackages/theme/src/base-select/index.less (3)
31-47: Consider adding browser support comments.The browser-specific code blocks would benefit from additional documentation explaining the specific issues they address.
&&__multiple:not(&__collapse-tags):not(&__filterable) &__tags > span { - // 兼容ie10-ie11 + // Fix for IE10-IE11: Ensures proper tag wrapping in multiple select mode @media screen and (-ms-high-contrast: active), (-ms-high-contrast: none) { display: flex; flex-wrap: wrap; justify-content: flex-start; align-content: flex-start; } - // 兼容edge + // Fix for Edge: Ensures proper tag layout and width calculation @supports (-ms-ime-align: auto) { width: 100%; display: flex;
243-245: Consider using more semantic class names.The class name 'hidden' is too generic. Consider using something more specific like 'select-tag-hidden'.
- .hidden { + .select-tag-hidden { visibility: hidden; }
377-378: Address the TODO comment about suffix button vertical alignment.The comment indicates that the suffix button's vertical alignment needs optimization.
Would you like me to help implement the vertical alignment fix or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/theme/src/base-select/index.less (1 hunks)
- packages/theme/src/base-select/vars.less (1 hunks)
- packages/vue/src/base-select/src/pc.vue (3 hunks)
🔇 Additional comments (7)
packages/theme/src/base-select/vars.less (4)
1-11: LGTM: License header is properly formatted.The copyright notices and MIT-style license disclaimer are correctly included.
21-28: LGTM: Icon properties are well-defined with appropriate state handling.The icon-related properties follow best practices by:
- Using semantic color variables
- Handling all interactive states (normal, hover, disabled)
- Maintaining consistent naming conventions
47-56: LGTM: Input and tags properties are well-structured.The properties use consistent spacing variables and follow the established naming conventions.
57-63: LGTM: Collapse button properties are properly defined.The collapse button properties use appropriate semantic color variables and consistent spacing.
packages/theme/src/base-select/index.less (2)
1-19: LGTM! File structure and imports are well organized.The file header, imports, and prefix variable definitions follow best practices.
1-401: Overall well-structured and maintainable stylesheet.The stylesheet demonstrates good practices with:
- Proper use of CSS variables for theming
- Well-organized component structure
- Good browser compatibility handling
While there are some minor suggestions for improvement, the implementation is solid and follows best practices.
packages/vue/src/base-select/src/pc.vue (1)
255-255: PropinputBoxTypeis correctly passed totiny-inputThe
inputBoxTypeprop is properly introduced and passed to thetiny-inputcomponent, enhancing configurability and allowing for consistent input box styling.
| // 边框激活色 | ||
| --tv-BaseSelect-input-border-color-active: var(--tv-color-border-active, #191919); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding border colors for other input states.
While the active state is handled, consider adding border colors for other common input states:
- Default state
- Hover state
- Focus state
- Error state
+ // Default border color
+ --tv-BaseSelect-input-border-color: var(--tv-color-border, #c2c2c2);
+ // Hover border color
+ --tv-BaseSelect-input-border-color-hover: var(--tv-color-border-hover, #808080);
// Active border color
--tv-BaseSelect-input-border-color-active: var(--tv-color-border-active, #191919);
+ // Focus border color
+ --tv-BaseSelect-input-border-color-focus: var(--tv-color-border-focus, #1476ff);
+ // Error border color
+ --tv-BaseSelect-input-border-color-error: var(--tv-color-error, #ff4d4f);📝 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-BaseSelect-input-border-color-active: var(--tv-color-border-active, #191919); | |
| // Default border color | |
| --tv-BaseSelect-input-border-color: var(--tv-color-border, #c2c2c2); | |
| // Hover border color | |
| --tv-BaseSelect-input-border-color-hover: var(--tv-color-border-hover, #808080); | |
| // Active border color | |
| --tv-BaseSelect-input-border-color-active: var(--tv-color-border-active, #191919); | |
| // Focus border color | |
| --tv-BaseSelect-input-border-color-focus: var(--tv-color-border-focus, #1476ff); | |
| // Error border color | |
| --tv-BaseSelect-input-border-color-error: var(--tv-color-error, #ff4d4f); |
| // 尾部图标距离输入框的垂直距离（默认尺寸） | ||
| --tv-BaseSelect-suffix-top: var(--tv-space-xl, 16px); | ||
| // 尾部图标距离输入框的垂直距离（中型尺寸） | ||
| --tv-BaseSelect-suffix-top-medium: 20px; | ||
| // 尾部图标距离输入框的垂直距离（小型尺寸） | ||
| --tv-BaseSelect-suffix-top-small: var(--tv-space-xl, 16px); | ||
| // 尾部图标距离输入框的垂直距离（迷你尺寸） | ||
| --tv-BaseSelect-suffix-top-mini: var(--tv-space-lg, 12px); | ||
| // 尾部图标右侧外间距 | ||
| --tv-BaseSelect-suffix-icon-margin-right: var(--tv-space-sm, 4px); | ||
| // 尾部文本外间距 | ||
| --tv-BaseSelect-suffix-txt-margin: 0 var(--tv-space-sm, 4px); | ||
| // 尾部文本色 | ||
| --tv-BaseSelect-suffix-text-color: var(--tv-color-text-weaken, #808080); |
There was a problem hiding this comment.
Replace hard-coded pixel value with a CSS variable.
The medium suffix top spacing uses a hard-coded value of 20px. Consider using a CSS variable for consistency with other spacing values.
- --tv-BaseSelect-suffix-top-medium: 20px;
+ --tv-BaseSelect-suffix-top-medium: var(--tv-space-2xl, 20px);📝 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-BaseSelect-suffix-top: var(--tv-space-xl, 16px); | |
| // 尾部图标距离输入框的垂直距离（中型尺寸） | |
| --tv-BaseSelect-suffix-top-medium: 20px; | |
| // 尾部图标距离输入框的垂直距离（小型尺寸） | |
| --tv-BaseSelect-suffix-top-small: var(--tv-space-xl, 16px); | |
| // 尾部图标距离输入框的垂直距离（迷你尺寸） | |
| --tv-BaseSelect-suffix-top-mini: var(--tv-space-lg, 12px); | |
| // 尾部图标右侧外间距 | |
| --tv-BaseSelect-suffix-icon-margin-right: var(--tv-space-sm, 4px); | |
| // 尾部文本外间距 | |
| --tv-BaseSelect-suffix-txt-margin: 0 var(--tv-space-sm, 4px); | |
| // 尾部文本色 | |
| --tv-BaseSelect-suffix-text-color: var(--tv-color-text-weaken, #808080); | |
| // 尾部图标距离输入框的垂直距离（默认尺寸） | |
| --tv-BaseSelect-suffix-top: var(--tv-space-xl, 16px); | |
| // 尾部图标距离输入框的垂直距离（中型尺寸） | |
| --tv-BaseSelect-suffix-top-medium: var(--tv-space-2xl, 20px); | |
| // 尾部图标距离输入框的垂直距离（小型尺寸） | |
| --tv-BaseSelect-suffix-top-small: var(--tv-space-xl, 16px); | |
| // 尾部图标距离输入框的垂直距离（迷你尺寸） | |
| --tv-BaseSelect-suffix-top-mini: var(--tv-space-lg, 12px); | |
| // 尾部图标右侧外间距 | |
| --tv-BaseSelect-suffix-icon-margin-right: var(--tv-space-sm, 4px); | |
| // 尾部文本外间距 | |
| --tv-BaseSelect-suffix-txt-margin: 0 var(--tv-space-sm, 4px); | |
| // 尾部文本色 | |
| --tv-BaseSelect-suffix-text-color: var(--tv-color-text-weaken, #808080); |
| z-index: 2; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using CSS variable for z-index value.
The hardcoded z-index value should be moved to a CSS variable for better maintainability.
- z-index: 2;
+ z-index: var(--tv-BaseSelect-expanded-z-index, 2);📝 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.
| z-index: 2; | |
| } | |
| z-index: var(--tv-BaseSelect-expanded-z-index, 2); | |
| } |
| max-width: 160px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using CSS variable for max-width value.
The hardcoded max-width value of 160px should be moved to a CSS variable for better maintainability and consistency.
- max-width: 160px;
+ max-width: var(--tv-BaseSelect-tag-max-width, 160px);📝 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.
| max-width: 160px; | |
| } | |
| max-width: var(--tv-BaseSelect-tag-max-width, 160px); | |
| } |
| transition: transform 0.3s; | ||
| transform: rotateZ(180deg); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing transition properties.
The transition could be more comprehensive to include other animated properties.
- transition: transform 0.3s;
+ transition: transform 0.3s, fill 0.3s;📝 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.
| transition: transform 0.3s; | |
| transform: rotateZ(180deg); | |
| transition: transform 0.3s, fill 0.3s; | |
| transform: rotateZ(180deg); |
| @@ -26,8 +26,7 @@ | |||
| clickExpand ? 'is-click-expand' : '', | |||
| state.showCollapseTag ? 'collapse-tag-clicked' : '', | |||
| state.selectDisabled ? 'is-disabled' : '', | |||
| $parent.$attrs.class, | |||
| inputBoxType === 'underline' ? 'tiny-base-select__underline' : '' | |||
| $parent.$attrs.class | |||
There was a problem hiding this comment.
Avoid directly accessing $parent in the template
Using $parent.$attrs.class in the template can lead to tight coupling between components and breaks the encapsulation of component design. It is recommended to pass necessary data through props or use provide/inject for better maintainability and reusability.
Apply this diff to refactor the code:
- $parent.$attrs.class
+ parentClassThen, in your component's props, add:
props: {
// existing props
parentClass: {
type: String,
default: ''
}
}And in the parent component, pass the class attribute:
<base-select :class="..." :parent-class="$attrs.class" ...></base-select>| @@ -548,6 +548,7 @@ | |||
| import TinyCheckbox from '@opentiny/vue-checkbox' | |||
|
|
|||
| import '@opentiny/vue-theme/select/index.less' | |||
| import '@opentiny/vue-theme/base-select/index.less' | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response:
The base-select style import path is correct but needs to be updated
The current import path '@opentiny/vue-theme/base-select/index.less' should be changed to use the theme-saas package instead, as the styles are located in packages/theme-saas/src/base-select/index.less. The vue-theme package does not exist in the codebase.
- Update the import statement to:
import '@opentiny/theme-saas/src/base-select/index.less'
🔗 Analysis chain
Confirm the import path for base-select styles
The import statement for base-select styles ensures that the component has the necessary styles applied. Please verify that the path '@opentiny/vue-theme/base-select/index.less' is correct and that it does not duplicate style imports.
You can run the following script to check for duplicate imports of the base-select styles:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for multiple imports of base-select styles in the codebase.
# Test: Search for all import statements of base-select styles.
# Expect: Only one import statement per component or as intended.
rg -t vue -t js -F "import '@opentiny/vue-theme/base-select/index.less'"
Length of output: 108
Script:
#!/bin/bash
# Let's try a different approach to find the style imports
# 1. First check if the path exists
fd "base-select" --type f
# 2. Then search for any imports containing base-select without file type restrictions
rg "import.*base-select.*less"
# 3. Also check for the theme package structure
fd --type f --full-path ".*vue-theme.*base-select"
Length of output: 507
Script:
#!/bin/bash
# Let's verify the theme structure and base-select style files
# 1. Check for base-select style files in theme packages
fd "index.less" --type f --parent-directory "base-select"
# 2. Check the content of theme-saas base-select styles to understand the structure
rg -g "base-select/*.less" -l ""
# 3. Look for any other potential style imports for base-select
rg -g "*.{vue,js}" "base-select.*\.less"
Length of output: 661
Script:
#!/bin/bash
# Let's try one final check with simpler commands
# 1. Find all less files related to base-select
fd -e less -g "*base-select*"
# 2. Check the theme package structure
fd . "packages/theme-saas"
# 3. Check the theme package structure for vue-theme
fd . "packages/vue-theme"
Length of output: 45692
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
New Features
searchable,showEmptyImage, and more.Bug Fixes
Documentation