refactor(select): [select] refactor theme vars for select #2232
Conversation
WalkthroughThe changes in this pull request involve updates to the event handling tests for a selection component and modifications to the CSS and properties of the select component. Specifically, the locator for the close icon in the tests has been altered, and various CSS classes and variables have been standardized and adjusted for better maintainability. Additionally, new properties and event emissions have been introduced in the component's Vue file to enhance its functionality and configurability. Changes
Possibly related PRs
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 (
|
![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 (2)
packages/theme/src/select/index.less (1)
193-201: Address TODO and approve CSS variable usageThe TODO comment suggests that the handling of disabled text in multi-select scenarios needs evaluation. Please review and address this TODO item.
The use of CSS variables for color and margins in the disabled state improves maintainability and theming capabilities. This change is approved.
Would you like assistance in evaluating the disabled text display in multi-select scenarios?
packages/theme-saas/src/select/index.less (1)
Line range hint
338-344: Inconsistent class name: Replace.tiny-select__closewith.@{select-prefix-cls}__closeThe selector
.tiny-select__closeshould be updated to use the class prefix variable.@{select-prefix-cls}__closefor consistency throughout the file.Apply this diff to fix the inconsistency:
- .tiny-select__close { + .@{select-prefix-cls}__close { @apply mr-2; @apply fill-color-text-placeholder; &:hover { @apply fill-color-icon-primary; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- examples/sites/demos/pc/app/select/events.spec.ts (2 hunks)
- packages/theme-saas/src/select/index.less (2 hunks)
- packages/theme/src/select/index.less (11 hunks)
- packages/theme/src/select/vars.less (1 hunks)
- packages/vue/src/select/src/pc.vue (5 hunks)
🧰 Additional context used
🔇 Additional comments (18)
examples/sites/demos/pc/app/select/events.spec.ts (3)
68-68: LGTM! Consistent change across tests.The update to the close icon selector in the multiple selection test is consistent with the change made in the single selection test. This ensures that both test cases are using the same updated class name for the close icon.
Line range hint
1-72: Summary: Consistent updates to select component close icon selectorsThe changes in this file are part of a larger refactoring effort for the select component. The updates to the close icon selectors in both single and multiple selection tests improve consistency in naming conventions without altering the functional behavior of the tests. These modifications align well with the PR objective of refactoring theme variables for the select component.
29-29: LGTM! Verify CSS changes across the project.The update to the close icon selector from
.tiny-select__caret.icon-closeto.tiny-select__caret.tiny-select__closelooks good. This change aligns with the PR objective of refactoring theme variables for the select component.To ensure consistency, let's verify if this class name change has been applied across the project:
✅ Verification successful
Selector update verified.
The update to the close icon selector from
.tiny-select__caret.icon-closeto.tiny-select__caret.tiny-select__closehas been successfully applied to the select component. No unintended impacts were found in other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining occurrences of the old class and confirm the usage of the new class. # Test 1: Search for any remaining occurrences of the old class echo "Searching for any remaining occurrences of 'icon-close' class:" rg --type css --type vue --type html 'icon-close' # Test 2: Confirm the usage of the new class echo "Confirming the usage of the new 'tiny-select__close' class:" rg --type css --type vue --type html 'tiny-select__close'Length of output: 439
Script:
#!/bin/bash # Description: Check for any remaining occurrences of the old class and confirm the usage of the new class. # Test 1: Search for any remaining occurrences of the old class echo "Searching for any remaining occurrences of 'icon-close' class:" rg 'icon-close' --glob '*.css' --glob '*.vue' --glob '*.html' # Test 2: Confirm the usage of the new class echo "Confirming the usage of the new 'tiny-select__close' class:" rg 'tiny-select__close' --glob '*.css' --glob '*.vue' --glob '*.html'Length of output: 7911
packages/theme/src/select/index.less (6)
107-107: LGTM: Improved maintainability with CSS variableThe use of
var(--ti-Select-tags-padding)for padding improves maintainability and allows for easier theming. This change enhances consistency across the component.
143-145: LGTM: Enhanced theming capabilitiesThe use of CSS variables for color, font-size, and height improves the theming capabilities of the select input. This change enhances maintainability and ensures consistency across the component.
152-152: LGTM: Consistent sizing with CSS variablesThe use of CSS variables for different select sizes (mini, small, medium) enhances consistency and maintainability. This change allows for easier theming and adjustments of select heights across different size variants.
Also applies to: 156-156, 160-160
172-172: LGTM: Improved tag margin consistencyThe use of
var(--ti-Select-tag-margin)for tag margins enhances consistency and maintainability. This change allows for easier theming and adjustments of tag spacing within the select component.
215-222: LGTM: Enhanced theming for collapse text and iconThe use of CSS variables for margin, font-size, color, and fill properties of the collapse text and icon improves consistency and maintainability. This change allows for easier theming and adjustments of these elements within the select component.
245-245: LGTM: Improved tag height consistencyThe use of
var(--ti-Select-tags-height)for tag container height enhances consistency and maintainability. This change allows for easier theming and adjustments of tag container height within the select component.packages/theme/src/select/vars.less (2)
14-55: CSS variable refactoring enhances consistency and maintainabilityThe refactoring of CSS variables to use the
--ti-Select-prefix standardizes the naming convention, improving code clarity and maintainability. This consistent approach will make it easier to manage theme variables across the select component.
31-37:⚠️ Potential issuePotential inconsistency in height variables
The variables
--ti-Select-heightand--ti-Select-height-mediumboth have the default value of32px. Specifically:
--ti-Select-heightis set tovar(--tv-size-height-md, 32px).--ti-Select-height-mediumis set tovar(--tv-size-height-lg, 32px).Since both resolve to
32px, this might cause confusion when styling different sizes of the select component. Typically, the medium size (--ti-Select-height-medium) should have a larger value than the default size (--ti-Select-height).Please verify the values of
--tv-size-height-mdand--tv-size-height-lgto ensure they are correctly defined. If they are intended to be different, consider adjusting them to reflect the appropriate sizes.packages/theme-saas/src/select/index.less (2)
Line range hint
2-4: Good use of variables for class prefixesDefining variables
@select-prefix-cls,@input-prefix-cls, and@tag-prefix-clsimproves maintainability and consistency across the stylesheet.
64-66: Consistent use of class prefix variable in selectorsUpdating the class name to
.@{select-prefix-cls}__closealigns with the new prefix variable convention, enhancing consistency.packages/vue/src/select/src/pc.vue (5)
28-28: Approved: Add 'is-disabled' class based on 'state.isDisabled'.Adding the 'is-disabled' class when
state.isDisabledis true ensures that the select component correctly reflects its disabled state in the UI.
95-95: Approved: Disable tag when item or select is disabled.Setting
:disabled="state.selected[0].disabled || state.isDisabled"ensures the tag component respects both the individual item's disabled state and the overall select component's disabled state.
129-129: Approved: Disable additional tags count when select is disabled.Applying
:disabled="state.isDisabled"to the tag displaying the count of additional selected items ensures consistent behavior when the select component is disabled.
295-295: Approved: Pass 'inputBoxType' prop to 'tiny-input' component.Including
:inputBoxType="inputBoxType"allows the input component to adjust its behavior or styling based on the specified input box type.
337-338: Approved: Update close icon with 'tiny-select__close' class.Using the class
tiny-select__closefor the close icon ensures consistency in styling and aligns with the component's CSS conventions.
| @@ -341,7 +337,7 @@ | |||
| } | |||
|
|
|||
| &__suffix { | |||
| display: var(--ti-select-suffix-display); | |||
| display: 'inline-block'; | |||
There was a problem hiding this comment.
Fix display property and approve CSS variable usage
The display property value should not be in quotes. Please remove the quotes around 'inline-block' to prevent potential issues:
- display: 'inline-block';
+ display: inline-block;The use of CSS variables for various properties (padding-right, fill, font-size, color, margin) improves maintainability and theming capabilities. These changes are approved.
Also applies to: 349-349, 372-373, 379-379, 389-391, 396-396, 411-411
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/select/vars.less (1)
46-47: Correct typo in comment: change "右测" to "右侧"There's a typographical error in the comment on line 46. The word "右测" should be "右侧", meaning "right side".
Apply this diff to fix the typo:
- // suffix 图标右测外间距 + // suffix 图标右侧外间距
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/theme/src/select/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/theme/src/select/vars.less (1)
14-55: Refactored variables enhance consistency and maintainabilityThe variables have been successfully refactored to use the standardized
--ti-Select-prefix. This change improves consistency across the codebase and makes the styles easier to maintain.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/theme/src/select/index.less (2)
193-201: Approved changes with a note on the TODOThe use of CSS variables for color and margins in the disabled state improves maintainability and theming capabilities. These changes are approved.
However, there's a TODO comment regarding the display of disabled text in multi-select scenarios. This should be addressed in a future update.
Would you like me to create a GitHub issue to track the TODO item for evaluating the display of disabled text in multi-select scenarios?
Line range hint
340-349: Approve CSS variable usage with a note on display propertyThe use of CSS variables for various properties (padding-right, fill, font-size, color, margin) improves maintainability and theming capabilities. These changes are approved.
However, there's a potential issue with the display property on line 340:
- display: 'inline-block'; + display: inline-block;The display property value should not be in quotes. Please remove the quotes around 'inline-block' to prevent potential issues.
Also applies to: 372-373, 379-379, 389-391, 396-396, 411-411
packages/theme/src/select/vars.less (1)
14-55: Consider Adding English Translations for CommentsThe comments in the code are currently written in Chinese (e.g.,
// 文本色). For better accessibility and collaboration among international contributors, consider providing English translations or adding comments in both languages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/select/index.less (11 hunks)
- packages/theme/src/select/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
packages/theme/src/select/index.less (6)
107-107: LGTM: Improved maintainability with CSS variableThe use of
var(--tv-Select-tags-padding)for padding improves maintainability and allows for easier theming. This change is approved.
143-145: LGTM: Enhanced theming capabilitiesThe use of CSS variables for color, font-size, and height improves theming capabilities and maintainability. These changes are approved.
152-160: LGTM: Consistent sizing with CSS variablesThe use of CSS variables for height across different size variants (mini, small, medium) improves consistency and maintainability. These changes are approved.
172-172: LGTM: Improved tag margin consistencyThe use of
var(--tv-Select-tag-margin)for tag margins improves consistency and maintainability. This change is approved.
215-222: LGTM: Improved theming for collapse text and iconThe use of CSS variables for margin, font-size, color, and fill properties in the collapse text and icon improves consistency and maintainability. These changes are approved.
245-245: LGTM: Consistent tag height with CSS variableThe use of
var(--tv-Select-tags-height)for tag height improves consistency and maintainability. This change is approved.
| // 文本色 | ||
| --tv-Select-text-color: var(--tv-color-text, #191919); | ||
| // 多选禁用文本色 | ||
| --ti-select-tags-text-color-disabled: var(--ti-common-color-text-disabled, #c2c2c2); | ||
| // 选择器后缀图标显示状态 | ||
| --ti-select-suffix-display: 'inline-block'; | ||
| // 选择器右侧图标间距 | ||
| --ti-select-suffix-icon-margin-right: var(--ti-common-space-base, 4px); | ||
| // 选择器suffix字号 | ||
| --ti-select-suffix-font-size: var(--ti-common-font-size-base, 14px); | ||
| // 选择器suffix文本色 | ||
| --ti-select-suffix-text-color: var(--ti-common-color-text-weaken, #808080); | ||
| // 收起按钮文本与图标色(hide) | ||
| --ti-select-collapse-button-text-icon-color: var(--ti-common-color-text-link, #1476ff); | ||
| // 收起按钮图标左边距(hide) | ||
| --ti-select-collapse-button-icon-margin-left: var(--ti-common-space-2, 2px); | ||
| // 收起按钮字号(hide) | ||
| --ti-select-collapse-button-font-size: var(--ti-common-font-size-base, 14px); | ||
| --tv-Select-text-color-disabled: var(--tv-color-text-disabled, #c2c2c2); | ||
| // 默认字号 | ||
| --tv-Select-font-size: var(--tv-font-size-md, 14px); | ||
|
|
||
| // 图标色 | ||
| --tv-Select-icon-color: var(--tv-color-icon, #808080); | ||
| // 图标禁用色 | ||
| --tv-Select-icon-color-disabled: var(--tv-color-icon-disabled, #c2c2c2); | ||
| // 图标悬浮色 | ||
| --tv-Select-icon-color-hover: var(--tv-color-icon-hover, #191919); | ||
| // 图标尺寸 | ||
| --tv-Select-icon-size: var(--tv-icon-size, 16px); | ||
|
|
||
| // 选择器高度 | ||
| --tv-Select-height: var(--tv-size-height-md, 32px); | ||
| // 选择器高度(mini) | ||
| --tv-Select-height-mini: var(--tv-size-height-xs, 24px); | ||
| // 选择器高度(small) | ||
| --tv-Select-height-small: var(--tv-size-height-sm, 28px); | ||
| // 选择器高度(medium) | ||
| --tv-Select-height-medium: var(--tv-size-height-lg, 40px); | ||
|
|
||
| // 标签区域高度 | ||
| --tv-Select-tags-height: var(--tv-size-height-md, 32px); | ||
| // 标签区域内边距 | ||
| --tv-Select-tags-padding: var(--tv-space-xs, 2px) 0px var(--tv-space-xs, 2px) var(--tv-space-xs, 2px); | ||
| // 标签外边距 | ||
| --tv-Select-tag-margin: var(--tv-space-xs, 2px); | ||
|
|
||
| // suffix 图标右测外间距 | ||
| --tv-Select-suffix-icon-margin-right: var(--tv-space-sm, 4px); | ||
| // suffix 文本外间距 | ||
| --tv-Select-suffix-text-margin: 0 var(--tv-space-sm, 4px); | ||
| // suffix 文本色 | ||
| --tv-Select-suffix-text-color: var(--tv-color-text-weaken, #808080); | ||
| // 收起按钮文本色 | ||
| --tv-Select-collapse-text-color: var(--tv-color-text-active, #1476ff); | ||
| // 收起按钮外间距 | ||
| --tv-Select-collapse-margin: 0 var(--tv-space-xs, 2px); |
There was a problem hiding this comment.
💡 Codebase verification
Incomplete Refactoring of Theme Variables
The search revealed instances of the old variable prefix --ti-select- in the following files:
./packages/theme/src/select-dropdown/index.less(lines 30-178)./packages/theme/src/select-dropdown/vars.less(lines 15-80)./packages/theme/src/base-select/index.less(lines 107-398)./packages/theme/src/base-select/vars.less(lines 15-79)
Please update all references from --ti-select- to --tv-Select- to ensure consistency and prevent styling issues.
🔗 Analysis chain
Ensure All References to Variables Are Updated
The theme variables have been refactored from --ti-select- to --tv-Select-. Please verify that all references to the old variable names have been updated across the entire codebase to prevent any styling inconsistencies or broken references.
You can run the following script to check for any remaining instances of the old variable names:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for occurrences of '--ti-select-' in LESS and CSS files.
# Test: Expect no matches for '--ti-select-' in the codebase.
rg --type less --type css --fixed-string '--ti-select-' --line-number
Length of output: 517
Script:
#!/bin/bash
# Description: Search for occurrences of '--ti-select-' in LESS and CSS files.
# Test: Expect no matches for '--ti-select-' in the codebase.
rg --type less --type css --fixed-strings '--ti-select-' --line-number
Length of output: 485
Script:
#!/bin/bash
# Description: Search for occurrences of '--ti-select-' in LESS and CSS files.
# Test: Expect no matches for '--ti-select-' in the codebase.
rg --type less --type css --fixed-strings '--ti-select-' --line-number .
Length of output: 487
Script:
#!/bin/bash
# Description: Search for occurrences of '--ti-select-' in LESS and CSS files.
# Test: Expect no matches for '--ti-select-' in the codebase.
rg --type less --type css --fixed-strings --line-number -- '--ti-select-' .
Length of output: 21777
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
examples/sites/demos/pc/app/select/input-box-type.spec.ts (3)
12-16: LGTM! Consider adding a comment for clarity.The changes to the class name and CSS property checks are consistent with the refactoring of theme variables for the select component. The more specific check for
border-bottom-coloraccurately reflects the underline style.Consider adding a brief comment explaining the significance of these checks, e.g.:
// Verify underline style and color for default state await expect(select.locator('.tiny-input')).toHaveClass(/tiny-input-underline/) await expect(input).toHaveCSS('border-bottom-color', 'rgb(194, 194, 194)')
33-37: LGTM! Consider enhancing consistency in disabled state checks.The updates to class name and CSS property checks are consistent with the previous test case and align with the theme variable refactoring.
For improved consistency and clarity, consider grouping related checks together:
// Verify underline style and color for disabled state await expect(select.locator('.tiny-input')).toHaveClass(/tiny-input-underline/) await expect(input).toHaveCSS('border-bottom-color', 'rgb(219, 219, 219)') await expect(input).toHaveCSS('cursor', 'not-allowed') await expect(select.locator('svg')).toHaveCSS('fill', 'rgb(194, 194, 194)') // Verify disabled attribute const hasDisabled = await input.evaluate((input) => input.hasAttribute('disabled')) await expect(hasDisabled).toBe(true)
58-62: LGTM! Consider reorganizing checks for better readability.The updates to class name and CSS property checks are consistent with the previous test cases and align with the theme variable refactoring. The additional check for the multiple selection class is appropriate.
Consider reorganizing the checks for better readability and logical grouping:
// Verify initial state await expect(select.locator('.tiny-input')).toHaveClass(/tiny-input-underline/) await expect(input).toHaveCSS('border-bottom-color', 'rgb(194, 194, 194)') await expect(select.locator('.tiny-select__caret')).toHaveCSS('fill', 'rgb(128, 128, 128)') // Perform selection await select.click() await expect(dropdown).toBeVisible() await option.first().click() // Verify final state await expect(tag).toHaveCount(5) await expect(select.locator('.tiny-input')).toHaveClass(/tiny-input-underline/) await expect(select).toHaveClass(/tiny-select__multiple/)Also applies to: 70-71
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- examples/sites/demos/pc/app/select/input-box-type.spec.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
examples/sites/demos/pc/app/select/input-box-type.spec.ts (1)
Line range hint
1-71: Overall, the changes effectively refactor theme variables for the select component.The modifications to class names and CSS property checks across all test cases are consistent and align well with the PR objectives. These changes improve the specificity of the tests and maintain consistency across different states of the select component (default, disabled, and multiple selection).
Key improvements:
- Updated class name checks from 'tiny-select__underline' to 'tiny-input-underline'.
- More specific CSS property checks, focusing on 'border-bottom-color' instead of 'border-color'.
- Consistent structure across all test cases.
These changes enhance the maintainability and accuracy of the tests, ensuring they properly reflect the refactored theme variables of the select component.
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
Bug Fixes
Style
Chores