refactor(autocomplete): [autocomplete] modify variable names and add … #2263
Conversation
WalkthroughThe changes in this pull request involve substantial updates to the styling and structure of the autocomplete component across multiple LESS and Vue files. Key modifications include the removal of outdated CSS variable prefixes, the introduction of new variables for improved styling control, and enhancements to the suggestion rendering logic in the Vue component. The overall structure remains consistent, but the updates streamline the styling process and improve the clarity of the component's functionality. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/theme/src/autocomplete/vars.less (2)
14-23: Approve general styling variables with a suggestion.The new variables for line height, font size, text color, and icon size are well-structured and promote consistency by referencing global theme variables. The addition of a selected text color variable improves styling flexibility.
Consider adding a comment to explain the purpose of
--tv-Autocomplete-text-color-selected, such as:// Text color for selected items in the autocomplete list --tv-Autocomplete-text-color-selected: var(--tv-color-text-active, #1476ff);This would enhance code readability and maintainability.
34-37: Approve list item styling variables with suggestions for improvement.The new variables for list item styling (padding and hover background color) are good additions that provide specific control over the autocomplete list items.
Consider the following improvements:
- Replace the hardcoded padding value with a theme variable:
--tv-Autocomplete-li-padding: var(--tv-space-sm, 5.5px) var(--tv-space-xl, 16px);
- Add a variable for the active/selected item background color:
// Background color for active/selected list item --tv-Autocomplete-li-bg-color-active: var(--tv-color-bg-active, #e6f2ff);These changes would further enhance consistency and provide more styling flexibility.
packages/vue/src/autocomplete/src/pc.vue (3)
54-56: LGTM! Consider adding a comment for clarity.The changes to the class attribute improve flexibility and visual feedback. The removal of the redundant
tiny-autocompleteclass is a good optimization.Consider adding a brief comment explaining the purpose of the
popperClassprop for better maintainability::class="[ popperClass ? popperClass : '', // Custom classes for the popper { 'is-loading': !hideLoading && state.loading } ]"
83-86: LGTM! Consider handling object comparisons.The addition of the
selectedclass improves visual feedback for the selected item. This is a good enhancement to the user experience.If
modelValuecould be an object, consider using a more robust comparison::class="{ highlighted: state.highlightedIndex === index, selected: typeof modelValue === 'object' ? modelValue[valueKey] === item[valueKey] : modelValue === item[valueKey] }"This change would handle cases where
modelValueis an object while maintaining the current behavior for primitive values.
Line range hint
114-151: LGTM! Consider enhancing prop documentation.The addition of new props and emits enhances the component's functionality and aligns well with the template changes.
To improve maintainability and ease of use, consider adding JSDoc comments for the new props. For example:
/** * The key of the value field in the suggestion objects. * @default 'value' */ valueKey: { type: String, default: 'value' }, /** * Custom class name for the autocomplete dropdown. */ popperClass: String, // ... (continue for other new props)This documentation will help other developers understand the purpose and expected values for each prop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/theme/src/autocomplete/index.less (2 hunks)
- packages/theme/src/autocomplete/vars.less (1 hunks)
- packages/theme/src/base/vars.less (1 hunks)
- packages/vue/src/autocomplete/src/pc.vue (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/theme/src/base/vars.less
🧰 Additional context used
🔇 Additional comments (9)
packages/theme/src/autocomplete/vars.less (3)
25-32: Approve panel styling variables.The new variables for panel styling (margin, border radius, background color, and box shadow) are well-defined and reference appropriate global theme variables. This approach ensures consistency with the overall theme while providing specific control over the autocomplete panel's appearance.
The use of a sophisticated box shadow variable (
--tv-shadow-3-down) is particularly noteworthy, as it allows for easy adjustment of the panel's depth perception.
14-37: Address overall changes and their potential impact.The refactoring of autocomplete variables introduces several significant changes:
- The naming convention has changed from
--ti-to--tv-.- The new set of variables is more concise but relies more on global theme variables.
- Some specific styling options (e.g., divider color) have been removed.
These changes improve overall consistency and simplify the theming system. However, they may require updates in other files and could impact existing customizations. Please ensure that:
- All files using the old
--ti-autocomplete-*variables are updated to use the new--tv-Autocomplete-*variables.- The documentation is updated to reflect these changes.
- A migration guide is provided for users who may have customized the removed variables.
Run the following script to check for usage of the old variable prefix in the codebase:
#!/bin/bash # Description: Check for usage of old autocomplete variable prefix # Test: Search for usage of old variable prefix rg --type less --type vue '\-\-ti\-autocomplete\-'If any results are found, update those occurrences to use the new variable names or add appropriate fallback values.
14-37:⚠️ Potential issueAddress potential impact of removed variables.
Several variables have been removed without direct replacements, including:
--ti-autocomplete-suggestion-border-color--ti-autocomplete-li-divider-border-color--ti-autocomplete-loading-text-color--ti-autocomplete-dropdown-gapThese removals might impact existing customizations. Please verify if these styles are still necessary and consider either:
- Adding back essential variables that are still needed.
- Documenting the changes in the migration guide if these styles are intentionally removed.
Run the following script to check for usage of the removed variables in the codebase:
If any results are found, consider addressing those usages or adding back the necessary variables.
packages/vue/src/autocomplete/src/pc.vue (1)
76-76: LGTM! Good use of self-closing tag.The change to a self-closing tag for the
icon-loading-shadowcomponent is a good practice for components without content. It improves code readability and follows Vue 3 conventions.packages/theme/src/autocomplete/index.less (5)
28-36: Verify redundancy between mixin and explicit variable definitionsThe addition of
.component-css-vars-autocomplete();mixin and the subsequent explicit definitions of CSS variables within&-suggestionmay introduce redundancy. Please ensure that the mixin does not already include these variables to avoid conflicting or duplicate styles.
41-44: Confirm scrollbar view styling adjustmentsApplying
margin: 0;andpadding: 8px 0;to.tiny-scrollbar__viewalters the spacing within the scrollbar. Verify that these changes align with the design requirements and do not negatively impact the user experience or overlap with existing scrollbar styles.
58-58: Consistent hover state styling for list itemsSetting the
background-colortovar(--tv-Autocomplete-li-bg-color-hover)for the hover and highlighted states ensures consistent theming across the component. This enhances the visual feedback for users interacting with the suggestion list.
62-62: Apply theme variable for selected text colorUsing
color: var(--tv-Autocomplete-text-color-selected);for the.selectedclass promotes consistency with the overall theme and allows for easier customization.
68-69: Use CSS variable for icon sizingSetting the icon size with
font-size: var(--tv-Autocomplete-icon-size);in.tiny-svgensures that icon dimensions are controlled by theme variables, allowing for consistent scaling across different components.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/theme/src/autocomplete/index.less (2)
43-43: Consider using CSS variables for padding.The padding for
.tiny-scrollbar__viewis set to8px 0;. Using a CSS variable for padding would enhance consistency and make future adjustments easier.Apply this diff to use a CSS variable:
- padding: 8px 0; + padding: var(--tv-Autocomplete-scrollbar-padding);Ensure that
--tv-Autocomplete-scrollbar-paddingis defined accordingly.
73-73: Adjust icon size using a CSS variable for consistency.The font size for
.tiny-svgis set usingvar(--tv-Autocomplete-icon-size);. Confirm that this variable is defined and consider using it consistently across similar icons for uniformity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/autocomplete/index.less (2 hunks)
- packages/vue/src/autocomplete/src/pc.vue (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vue/src/autocomplete/src/pc.vue
🧰 Additional context used
🔇 Additional comments (4)
packages/theme/src/autocomplete/index.less (4)
28-29: Verify the inclusion of the mixin.component-css-vars-autocomplete().Ensure that the mixin
.component-css-vars-autocomplete()is defined and properly imported so that all CSS variable definitions are correctly applied to the&-suggestionclass.
62-62: Ensure hover state background color variable is defined.The background color for the highlighted state uses
var(--tv-Autocomplete-li-bg-color-hover);. Verify that this variable is properly defined to avoid any unexpected styling issues on hover.
66-66: Confirm selected text color variable is defined.The selected list item's text color is set to
var(--tv-Autocomplete-text-color-selected);. Please ensure this variable is defined to maintain the intended styling for selected items.
30-36:⚠️ Potential issueConfirm all new CSS variables are defined.
The code introduces new CSS variables such as
--tv-Autocomplete-panel-border-radius,--tv-Autocomplete-panel-bg-color, and others. Please verify that these variables are defined invars.lessor the appropriate variables file to prevent potential styling issues.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/autocomplete/index.less (2 hunks)
- packages/theme/src/autocomplete/vars.less (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/autocomplete/index.less
🧰 Additional context used
🔇 Additional comments (5)
packages/theme/src/autocomplete/vars.less (5)
13-14: Function name change and new variable naming conventionThe function name has been updated from
.component-css-vars-autocomplete()to.inject-Autocomplete-vars(), which better reflects its purpose. The new naming convention for variables (using--tv-prefix) is more consistent and aligns with modern CSS practices.
15-21: Text-related variables improvementsThe new variables for line height, font size, and text color provide more granular control over the text appearance. The addition of a separate variable for selected text color (
--tv-Autocomplete-text-color-selected) is a good improvement for accessibility and user experience.
22-23: Icon size variable additionThe introduction of
--tv-Autocomplete-icon-sizeis a good addition, allowing for consistent icon sizing across the component.
25-32: Panel-related variables enhancementsThe new panel-related variables provide more detailed control over the appearance of the Autocomplete panel. The addition of
--tv-Autocomplete-panel-box-shadowis particularly useful for creating depth and improving visual hierarchy.
13-37: Overall improvement in variable structure and namingThe refactoring of CSS variables for the Autocomplete component represents a significant improvement in organization, naming conventions, and granularity of control. These changes align with modern CSS best practices and will likely improve the maintainability of the codebase.
However, it's important to note that these changes may have a significant impact on existing implementations that rely on the old variable names.
To ensure a smooth transition, please consider the following:
- Update the component's documentation to reflect the new variable names and their purposes.
- Provide a migration guide for users updating from the previous version.
- Consider adding deprecation warnings for the old variable names if backward compatibility is a concern.
To verify the impact of these changes, you can run the following script to check for any remaining usage of the old variable names:
This will help ensure that all occurrences of the old variable names have been updated throughout the codebase.
| // 列表项内间距 | ||
| --tv-Autocomplete-li-padding: 5.5px var(--tv-space-xl, 16px); | ||
| // 列表项悬浮背景色 | ||
| --tv-Autocomplete-li-bg-color-hover: var(--tv-color-bg-hover, #f5f5f5); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
List item-related variables refinement
The new variables for list item padding and hover background color offer more precise control over the appearance of individual items in the Autocomplete list.
However, there's an opportunity for improvement:
Consider adding a variable for the selected item's background color to provide more flexibility in styling. For example:
// 列表项悬浮背景色
--tv-Autocomplete-li-bg-color-hover: var(--tv-color-bg-hover, #f5f5f5);
+ // 列表项选中背景色
+ --tv-Autocomplete-li-bg-color-selected: var(--tv-color-bg-active, #e6f2ff);This addition would allow for distinct styling of hovered and selected items, enhancing user experience.
📝 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-Autocomplete-li-padding: 5.5px var(--tv-space-xl, 16px); | |
| // 列表项悬浮背景色 | |
| --tv-Autocomplete-li-bg-color-hover: var(--tv-color-bg-hover, #f5f5f5); | |
| // 列表项内间距 | |
| --tv-Autocomplete-li-padding: 5.5px var(--tv-space-xl, 16px); | |
| // 列表项悬浮背景色 | |
| --tv-Autocomplete-li-bg-color-hover: var(--tv-color-bg-hover, #f5f5f5); | |
| // 列表项选中背景色 | |
| --tv-Autocomplete-li-bg-color-selected: var(--tv-color-bg-active, #e6f2ff); |
…refactor styles
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