refactor(select): [select] refactor theme vars and style for select-dropdown/option/option-group #2254
Conversation
WalkthroughThe pull request introduces significant modifications across several components, particularly the Changes
Possibly related PRs
Suggested labels
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: 7
🧹 Outside diff range and nitpick comments (31)
packages/theme/src/option-group/index.less (1)
27-27: Consider using a CSS variable for padding-bottom.The change to a fixed value of 0px simplifies the styling. However, for consistency with the overall theming approach and to maintain flexibility, consider using a CSS variable instead.
You could define a new variable like
--tv-OptionGroup-padding-bottomand use it here:- padding-bottom: 0px; + padding-bottom: var(--tv-OptionGroup-padding-bottom, 0px);This maintains the current behavior while allowing for easy customization in the future.
packages/theme/src/option/vars.less (2)
27-36: Enhanced icon styling control with clear naming.The new icon-related variables offer improved styling flexibility:
- Separate variables for icon size and various checkbox states.
- Clear and consistent naming (e.g.,
--tv-Option-icon-color-unchecked).These changes allow for more precise theming of icons and checkboxes.
Consider adding a comment to explain the purpose of
--tv-Option-icon-bg-color-disabled, as its usage might not be immediately clear from the name alone.
38-43: Improved layout control with new variables.The new layout-related variables enhance control over option appearance:
- Separate variables for option height and icon margin.
- Combined padding variable for simplicity.
These changes provide better control over the option layout.
Consider separating the horizontal and vertical padding into two variables (e.g.,
--tv-Option-padding-verticaland--tv-Option-padding-horizontal) to allow for more flexible styling, especially for cases where different vertical and horizontal padding might be desired.packages/vue/src/option/src/mobile-first.vue (1)
Line range hint
67-67: Approve new props with documentation suggestion.The addition of new props ('value', 'label', 'created', 'disabled', 'events', 'visible', 'highlightClass', 'required') enhances the component's flexibility and aligns with expected functionality for a select option.
Please ensure that these new props are properly documented in the component's API documentation. Additionally, consider adding JSDoc comments for these props in the component file itself.
Example:
props: [ ...props, /** * The value of the option. */ 'value', /** * The label to display for the option. */ 'label', // ... (add comments for other props) ],packages/theme/src/base/vars.less (1)
294-294: LGTM! Consider adding a comment for clarity.The new variable
--tv-color-icon-checked-disabledis a valuable addition to the design system, addressing the specific state of a checked and disabled icon. It's well-named and uses an existing color variable, maintaining consistency.For improved clarity, consider adding a brief comment explaining the use case, similar to other variables in this section. For example:
- --tv-color-icon-checked-disabled:var(--tv-base-color-common-6); // #dbdbdb 图标禁用色-1 checkbox选中且禁用 + --tv-color-icon-checked-disabled: var(--tv-base-color-common-6); // #dbdbdb 图标禁用色-1 checkbox选中且禁用状态packages/theme/src/select-dropdown/index.less (6)
44-44: Use English for code commentsThe comment
// 顶部新增is in Chinese. Please use English for code comments to ensure clarity for all contributors.
74-74: Use English for code commentsThe comment
// 面板搜索框is in Chinese. Consistency in language within code comments aids in better collaboration. Please convert it to English.
135-135: Use English for code commentsThe comment
// 虚拟滚动is in Chinese. Rewriting it in English will improve readability for all team members.
151-151: Use English for code commentsThe comment
// 空数据is in Chinese. Please translate it to English to maintain consistency.
166-166: Use English for code commentsThe comment
// 加载中is in Chinese. Updating it to English will help in universal understanding.
185-185: Use English for code commentsThe comment
// tiny 新增,空数据图片is in Chinese. Converting code comments to English ensures clarity across the development team.packages/vue/src/base-select/src/pc.vue (5)
Line range hint
433-443: Simplify complexv-ifcondition for better readabilityThe
v-ifcondition on lines 433-434 is quite complex:v-if="multiple && showCheck && showAlloption && !state.multipleLimit && !state.query && !remote"Consider refactoring this condition into a computed property with a descriptive name to enhance readability and maintainability.
Line range hint
440-440: Replace magic number-9with a named constantIn the
@mouseenterevent handler on line 440,state.hoverIndexis assigned the value-9:@mouseenter="state.hoverIndex = -9"Using magic numbers can reduce code clarity and make maintenance harder. Define a constant with a descriptive name to represent this value.
Line range hint
458-468: Simplify complexv-ifcondition for better readabilityThe
v-ifcondition on lines 458-464 is also complex:v-if=" multiple && showCheck && showAlloption && !state.multipleLimit && state.query && !state.emptyText && !remote "Refactoring this condition into a computed property will improve code readability and ease future maintenance.
Line range hint
466-466: Replace magic number-9with a named constantAgain, on line 466,
state.hoverIndexis set to-9:@mouseenter="state.hoverIndex = -9"Consider defining a meaningful constant for
-9to enhance code clarity.
Line range hint
433-468: Refactor duplicate<li>elements to reduce code duplicationThe
<li>elements from lines 433-443 and 458-468 have similar structures with minor differences in conditions and class bindings. Extracting common logic into a reusable component or method can reduce duplication and improve maintainability.packages/vue/src/select/src/pc.vue (3)
Line range hint
488-498: Simplify the conditional logic inv-ifdirectiveThe condition in the
v-ifdirective uses the bitwise NOT operator withindexOf, which can be less readable:v-if="!optimization && !~['grid', 'tree'].indexOf(renderType)"Consider simplifying it for better readability by using
includes:v-if="!optimization && !['grid', 'tree'].includes(renderType)"This makes the condition more straightforward by checking if
renderTypeis not included in the array['grid', 'tree'].
Line range hint
506-515: Replace magic numbers with named constants for better clarityThe use of
-9as a special index value can be confusing and reduces code readability::class="[ { hover: state.hoverIndex === -9 && state.selectCls !== 'checked-sur' }, { selected: state.selectCls === 'checked-sur' } ]"Define a constant to represent this special index:
const ALL_OPTION_INDEX = -9;And update the code accordingly:
:class="[ - { - hover: state.hoverIndex === -9 && state.selectCls !== 'checked-sur' - }, + { + hover: state.hoverIndex === ALL_OPTION_INDEX && state.selectCls !== 'checked-sur' + }, { selected: state.selectCls === 'checked-sur' } ]"This improves maintainability by making the code's intent clearer.
Line range hint
531-542: Avoid using magic numbers; introduce a named constantSimilar to the previous case,
-9is used again as a special value::class="[ { hover: state.hoverIndex === -9 && state.filteredSelectCls !== 'checked-sur' }, { selected: state.filteredSelectCls === 'checked-sur' } ]"Define a constant for clarity:
const ALL_OPTION_INDEX = -9;Update the code:
:class="[ - { - hover: state.hoverIndex === -9 && state.filteredSelectCls !== 'checked-sur' - }, + { + hover: state.hoverIndex === ALL_OPTION_INDEX && state.filteredSelectCls !== 'checked-sur' + }, { selected: state.filteredSelectCls === 'checked-sur' } ]"This enhances code readability and makes future maintenance easier.
packages/renderless/src/base-select/index.ts (8)
Line range hint
684-686: Translate Non-English Comments to EnglishThe comment
// tiny 新增 修复select组件，创建条目选中再创建选中，还是上一次的数据is in Chinese. To maintain consistency and readability for all developers, please translate comments into English.
Line range hint
426-428: Translate Non-English Comments to EnglishThe comment
// tiny 新增： 表单校验不能异步，否则弹窗中嵌套表单会出现弹窗关闭后再出现校验提示的bugis in Chinese. Please translate it into English to ensure that all team members can understand it.
Line range hint
1720-1729: Review Equality Checks inclearNoMatchValueIn the
clearNoMatchValuemethod, the condition comparingprops.modelValueandnewModelValuemay not work as intended if the values are objects or arrays with different references but identical contents. Consider using a deep equality check to ensure accurate comparisons.Apply this diff to use deep equality checking:
export const clearNoMatchValue = ({ props, emit }) => (newModelValue) => { if (!props.clearNoMatchValue) { return } - if ( - (props.multiple && props.modelValue.length !== newModelValue.length) || - (!props.multiple && props.modelValue !== newModelValue) - ) { + const valuesChanged = props.multiple + ? !isEqual(props.modelValue, newModelValue) + : !isEqual(props.modelValue, newModelValue) + if (valuesChanged) { emit('update:modelValue', newModelValue) } }
Line range hint
1761-1766: Re-evaluate State Update Logic inupdateModelValueIn
updateModelValue, when on mobile devices (state.device === 'mb') withprops.multipleand!needUpdate, directly assigningstate.modelValue = valuebypasses Vue's reactivity system. This may prevent watchers and computed properties from updating. Consider usingthis.$setor ensuring updates trigger reactivity.Apply this diff to ensure reactivity:
export const updateModelValue = ({ props, emit, state }) => (value, needUpdate) => { state.isClickChoose = true if (state.device === 'mb' && props.multiple && !needUpdate) { - state.modelValue = value + state.modelValue = [...value] } else { emit('update:modelValue', value) } }
Line range hint
938-949: Use Modern Clipboard API inhandleCopyClickThe
handleCopyClickmethod usesdocument.execCommand('copy'), which is deprecated and may not be supported in future browsers. It also manipulates the DOM by creating and removing input elements. Consider using the modern Clipboard API for better reliability and cleaner code.Refactor the method to use the Clipboard API:
export const handleCopyClick = ({ parent, props, state }) => () => { - const input = document.createElement('input') - - input.style.height = 0 - input.style.border = 'none' - input.style.position = 'absolute' - - parent.$el.appendChild(input) - - input.value = state.selected - .map((item) => (item.state ? item.state.currentLabel : item.currentLabel)) - .join(props.textSplit) - - input.select() - document.execCommand('copy') - parent.$el.removeChild(input) + const textToCopy = state.selected + .map((item) => (item.state ? item.state.currentLabel : item.currentLabel)) + .join(props.textSplit) + navigator.clipboard.writeText(textToCopy).catch((err) => { + console.error('Could not copy text: ', err) + }) }
Line range hint
1710-1713: Ensure Debounce Delay is Appropriately ConfiguredIn
handleDebouncedQueryChange,debounce(state.debounce, ...)is used, but thestate.debouncevalue is not initialized in the provided code. Ensure thatstate.debouncehas a valid number assigned to it before it's used, to prevent potential runtime errors.
Line range hint
684-686: Avoid Duplicated Code inhandleOptionSelectIn
handleOptionSelect, the logic for updatingstate.createdSelectedandstate.createdLabelwhenoption.createdis true seems similar to other parts of the code. Consider refactoring this logic into a separate method to avoid code duplication.
Line range hint
1710-1713: Initializestate.previousQueryAppropriately inclearSearchTextIn
clearSearchText,state.previousQueryis set toundefined. If other methods rely onstate.previousQuerybeing a string, this could lead to errors. Consider initializing it to an empty string instead.Apply this diff:
export const clearSearchText = ({ state, api }) => () => { state.query = '' - state.previousQuery = undefined + state.previousQuery = '' api.handleQueryChange(state.query) }packages/renderless/src/select/index.ts (4)
Line range hint
1980-1982: Typo in Function NamedebouncRquestThe function name
debouncRquestappears to be misspelled. It should likely bedebounceRequestto accurately reflect its purpose and maintain naming consistency.Apply this diff to correct the function name:
-export const debouncRquest = ({ api, state, props }) => +export const debounceRequest = ({ api, state, props }) =>
Line range hint
2065-2072: Redundant Custom Debounce Implementation inhandleDebouncedQueryChangeThe
handleDebouncedQueryChangefunction uses a custom debounce implementation. Since a debounced functiondebounceRequestalready exists, consider reusing it to avoid redundancy and ensure consistent behavior.Apply this diff to utilize the existing debounced function:
-export const handleDebouncedQueryChange = ({ state, api }) => - debounce(state.debounce, (value) => { - api.handleQueryChange(value) - }) +export const handleDebouncedQueryChange = ({ api }) => { + api.debounceRequest() +}
Line range hint
1020-1030: Missing Documentation forclearNoMatchValueFunctionThe newly added
clearNoMatchValuefunction lacks documentation. Adding a JSDoc comment will enhance code maintainability by explaining the function's purpose, parameters, and return value.Consider adding documentation like this:
/** * Clears the model value when there is no matching option. * @param newModelValue - The new model value to be set. */
Line range hint
1450-1465: Potential Overwrite of Event Parameter inhandleBlurIn the
handleBlurfunction, there is a possibility that theeventobject is overwritten or used incorrectly due to asynchronous operations withinsetTimeout. Ensure that theeventobject is correctly captured and used within the callback.Consider using a closure to capture the event:
export const handleBlur = ({ constants, dispatch, emit, state, designConfig }) => (event) => { + const blurEvent = event clearTimeout(state.timer) state.timer = setTimeout(() => { if (state.isSilentBlur) { state.isSilentBlur = false } else { - emit('blur', event) + emit('blur', blurEvent) } // existing code... }, 200) // existing code... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
- packages/renderless/src/base-select/index.ts (1 hunks)
- packages/renderless/src/select/index.ts (1 hunks)
- packages/theme/src/base/vars.less (1 hunks)
- packages/theme/src/option-group/index.less (2 hunks)
- packages/theme/src/option-group/vars.less (1 hunks)
- packages/theme/src/option/index.less (2 hunks)
- packages/theme/src/option/vars.less (1 hunks)
- packages/theme/src/select-dropdown/index.less (4 hunks)
- packages/theme/src/select-dropdown/vars.less (1 hunks)
- packages/vue/src/base-select/src/mobile-first.vue (2 hunks)
- packages/vue/src/base-select/src/pc.vue (2 hunks)
- packages/vue/src/option/src/mobile-first.vue (1 hunks)
- packages/vue/src/option/src/pc.vue (1 hunks)
- packages/vue/src/select/src/mobile-first.vue (2 hunks)
- packages/vue/src/select/src/pc.vue (6 hunks)
🧰 Additional context used
🔇 Additional comments (29)
packages/theme/src/option-group/vars.less (5)
14-19: Improved naming convention and fallback values for font-related variables.The new variables for font size, text color, and line height follow a more consistent naming pattern (
--tv-OptionGroup-*) and include fallback values. This change enhances maintainability and ensures compatibility.
21-24: Verify the intention behind setting divider height to 0px.The new divider border color variable is well-defined. However, setting
--tv-OptionGroup-height-dividerto 0px effectively removes the divider. Please confirm if this is intentional, as it represents a significant change from the previous implementation.
26-29: Improved naming and structure for title-related variables.The new variables for title padding and margin follow the consistent
--tv-OptionGroup-*naming pattern and maintain the use of fallback values. This refactoring enhances readability and maintainability of the code.
31-32: Consistent naming for first title margin variable.The new variable
--tv-OptionGroup-title-margin-top-firstfollows the established naming pattern and includes a fallback value. This change is in line with the overall refactoring effort and improves consistency.
14-32: Overall assessment: Successful refactoring with one point for verification.This refactoring of the option group CSS variables significantly improves consistency and maintainability. The new naming convention (
--tv-OptionGroup-*) is applied uniformly, and fallback values are consistently used. These changes align well with the PR objectives.Key improvements:
- Consistent naming pattern across all variables.
- Maintained use of fallback values for better compatibility.
- Reorganized structure that enhances readability.
One point requires verification:
- The divider height is set to 0px, which effectively removes the divider. Please confirm if this is intentional.
Once the divider height issue is addressed, this refactoring appears to be a solid improvement to the codebase.
packages/theme/src/option-group/index.less (4)
37-38: Excellent use of new CSS variables for the divider.The introduction of
--tv-OptionGroup-height-dividerand--tv-OptionGroup-border-color-dividerimproves theming consistency and customization capabilities. This change aligns well with the new naming convention and enhances the component's flexibility.
42-48: Great refactoring of the title class styling.The changes in the title class styling are well-executed:
- Consistent use of the new
tv-prefix for CSS variables.- Enhanced customization capabilities through the use of variables for all properties.
- Simplified styling by consolidating margin properties into a single variable.
These changes improve the overall maintainability and flexibility of the component's styling.
51-52: Good use of a specific variable for the first title's top margin.The introduction of
--tv-OptionGroup-title-margin-top-firstfor the first child title's top margin is a positive change. It aligns with the new naming convention and provides targeted customization for this specific use case.
Line range hint
27-52: Excellent refactoring of the option group styling.The changes in this file successfully achieve the PR's objective of refactoring theme variables and styles for the option group component. Key improvements include:
- Consistent application of the new
tv-prefix for CSS variables.- Enhanced customization capabilities through the use of variables for various properties.
- Simplified styling in some areas, such as consolidating margin properties.
- Improved specificity for certain styles, like the first child title's top margin.
These changes contribute to better maintainability, flexibility, and consistency in the theming system. The refactoring aligns well with modern CSS best practices and should make future updates and customizations easier.
packages/theme/src/option/vars.less (3)
14-25: Improved naming convention and granular control for text-related variables.The changes to font and text-related variables are well-structured and provide better control:
- Consistent naming convention using camelCase (e.g.,
--tv-Option-font-size).- Separate variables for different text states (normal, disabled, selected, highlight).
- Use of fallback values for better robustness.
These improvements enhance readability and maintainability of the theme.
44-45: Added hover state customization.The addition of
--tv-Option-bg-color-hoverallows for easy customization of the option's hover state, improving the overall theming capabilities.
14-45: Comprehensive refactoring of Option component variables.The changes represent a significant improvement in the organization and naming of CSS variables for the Option component:
- Consistent prefix change from
--ti-to--tv-.- More descriptive and granular variable names.
- Better organization of variables by purpose (text, icon, layout).
These changes align well with the PR objectives and improve the overall maintainability of the theme.
However, it's important to verify the impact of these changes:
Please ensure that all usages of the old variable names have been updated accordingly to prevent any breaking changes.
packages/vue/src/option/src/pc.vue (1)
21-21: Improved flexibility in icon stylingThe modification to the class binding for the icon component is a good improvement. It maintains the existing
tiny-svg-sizeclass while adding the dynamicstate.selectCls, allowing for more flexible styling based on the select state.This change aligns well with the PR's objective of refactoring theme variables and styles for select components. It should provide better control over the appearance of the checkbox icon in different states (e.g., checked, unchecked) without affecting its size.
packages/vue/src/option/src/mobile-first.vue (2)
Line range hint
1-70: Summary: Changes align with PR objectives.The modifications in this file, including the
data-tagattribute change and the addition of new props, are consistent with the PR's objective of refactoring theme variables and styles for select components. These changes enhance the component's flexibility and improve its naming consistency.To ensure a smooth integration of these changes:
- Update the component's documentation to reflect the new props and their usage.
- Verify that any code depending on the old
data-tagvalue is updated accordingly.- Consider adding unit tests for the new props to ensure they behave as expected.
22-22: Approve data-tag change with verification suggestion.The change from "tiny-select-dropdown-item" to "tiny-option" for the
data-tagattribute improves clarity and aligns with the component's purpose. This is a good refactoring step.Please verify that this change doesn't break any existing CSS selectors or JavaScript code that might be relying on the previous
data-tagvalue. Run the following script to check for any occurrences of the old tag:✅ Verification successful
No remaining instances of "tiny-select-dropdown-item" found.
The change does not affect existing CSS selectors or JavaScript code relying on the old
data-tagvalue.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for occurrences of the old data-tag value # Test: Search for the old data-tag value rg --type vue --type css --type javascript "tiny-select-dropdown-item"Length of output: 98
Script:
#!/bin/bash # Description: Search for occurrences of the old data-tag value # Test: Search for the old data-tag value rg "tiny-select-dropdown-item" -g "*.vue" -g "*.css" -g "*.js"Length of output: 62
packages/theme/src/option/index.less (2)
87-87: Verify Class Name 'memorize-highlight'The class name
memorize-highlightat line 87 may be a typo or might not accurately reflect its purpose. Please confirm whether this is the intended class name or if it should be corrected to something likememory-highlightorhighlightto maintain consistency and clarity in naming conventions.To check the usage of
memorize-highlightacross the codebase, you can run:#!/bin/bash # Description: Find all instances of 'memorize-highlight' to verify its usage. # Expected result: Ensure that the class name is consistently used and defined. rg 'memorize-highlight'
47-49: Verify Class Names 'checked-sur' and 'halfselect'In the selectors at lines 48-49:
.checked-sur.tiny-svg, .halfselect.tiny-svg {Please confirm that the class names
checked-surandhalfselectare correctly defined and used consistently throughout the codebase. Consistent naming helps maintain readability and reduces potential confusion.To verify the definitions and usage of these class names, consider running:
✅ Verification successful
Re-Verify Class Names 'checked-sur' and 'halfselect'
Please run the following script to accurately search for definitions and usages of
checked-surandhalfselectacross relevant file types:
Verified: Class Names 'checked-sur' and 'halfselect' are Correctly Defined and Used Consistently
The class names
checked-surandhalfselectare properly defined and consistently used within the Less files of the codebase, ensuring maintainable and clear styling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for definitions and usages of 'checked-sur' and 'halfselect'. # Expected result: Find all occurrences to ensure they are correctly implemented. rg 'class.*checked-sur' --type js,vue,less rg 'class.*halfselect' --type js,vue,lessLength of output: 1174
Script:
#!/bin/bash # Description: Search for definitions and usages of 'checked-sur' and 'halfselect'. # Ensures accurate patterns for different file types. # Expected result: Find all occurrences to confirm correct implementation. # Search in JS and Vue files using class keyword rg 'class\s+["'\''`]?[A-Za-z0-9\-]*checked-sur["'\''`]?' rg 'class\s+["'\''`]?[A-Za-z0-9\-]*halfselect["'\''`]?' # Search in Less files using dot notation rg '\.checked-sur' rg '\.halfselect'Length of output: 434
packages/theme/src/select-dropdown/index.less (3)
30-37: Variables updated for consistencyThe theme variables have been correctly updated to the new
--tv-prefixed variables, enhancing maintainability and consistency across the component.
75-75: Verify selector nesting in& &__searchThe selector
& &__searchmay not generate the intended CSS rules. Please verify that the nesting produces the correct selector for styling the search component.
147-147: Verify selector nesting in& &__wrapThe selector
& &__wrapmight not produce the desired CSS hierarchy. Ensure that the nesting aligns with the intended DOM structure.packages/vue/src/base-select/src/mobile-first.vue (2)
464-464: Ensure Consistent Update ofdata-tagAttributeSimilar to the previous change, the
data-tagattribute here is updated to"tiny-option". Please ensure consistency across the codebase by updating all instances.Use the following script to locate any remaining old references:
#!/bin/bash # Description: Search for the old data-tag attribute in the codebase. rg --type vue 'data-tag="tiny-select-dropdown-item"'
431-431: Ensure Consistent Update ofdata-tagAttributeThe
data-tagattribute has been changed to"tiny-option". Please verify that all references to the olddata-tag="tiny-select-dropdown-item"have been updated throughout the codebase to prevent any unintended side effects.You can run the following script to check for any remaining occurrences of the old
data-tag:✅ Verification successful
All references to
data-tag="tiny-select-dropdown-item"have been successfully updated throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the old data-tag attribute in the codebase. rg --type vue 'data-tag="tiny-select-dropdown-item"'Length of output: 82
Script:
#!/bin/bash # Description: Search for the old data-tag attribute in .vue files. rg 'data-tag="tiny-select-dropdown-item"' -g '*.vue'Length of output: 54
packages/vue/src/select/src/mobile-first.vue (2)
Line range hint
445-452: Renameddata-tagattribute totiny-optionThe update of the
data-tagattribute to"tiny-option"is consistent and aligns with the refactoring objectives.
Line range hint
478-485: Consistent update ofdata-tagtotiny-optionThe
data-tagattribute has been changed to"tiny-option"here as well, maintaining consistency throughout the component.packages/vue/src/select/src/pc.vue (1)
571-575: Duplicate condition inv-ifdirectiveThe
v-ifcondition seems to duplicate logic that may already be handled elsewhere:<div v-if="loadingText || slots.empty" class="tiny-select-dropdown__empty-wrap">Please ensure that this condition does not overlap with other conditions and that it's necessary. Check if
loadingTextandslots.emptyare correctly reflecting the component's state.packages/renderless/src/base-select/index.ts (2)
Line range hint
1710-1713: EnsureclearSearchTextDoes Not Disrupt Query LogicIn the
clearSearchTextmethod, resettingstate.queryandstate.previousQueryto empty values may impact components relying on these states. Verify that clearing these values does not cause unintended side effects in search functionality.
Line range hint
1626-1634: Verify Selector Update inqueryVisibleOptionsThe selector in
queryVisibleOptionshas been updated to[data-index]:not([style*="display: none"]). Ensure that this selector accurately selects all visible options, especially considering any dynamic styles that might hide options without usingdisplay: none.✅ Verification successful
Verification Successful: Selector Accurately Selects Visible Options
The updated selector
[data-index]:not([style*="display: none"])correctly targets all fully visible options. No elements are hidden usingvisibility: hiddenoropacity: 0that would affect the selector's accuracy.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other styles that hide options without using 'display: none' # Test: Search for CSS rules or inline styles that hide elements using 'visibility: hidden' or 'opacity: 0' rg --type css 'visibility:\s*hidden|opacity:\s*0' --glob '*.css' # Test: Search for inline styles in component templates that hide elements rg --type vue 'style=.*(visibility:\s*hidden|opacity:\s*0)'Length of output: 290
Script:
#!/bin/bash # Description: Re-verify styles that hide options without using 'display: none' # Test: Search for CSS rules that hide elements using 'visibility: hidden' or 'opacity: 0' rg --type css 'visibility:\s*hidden|opacity:\s*0' --glob '*.css' # Test: Search for inline styles in Vue component templates that hide elements rg --type html 'style=.*(visibility:\s*hidden|opacity:\s*0)' --glob '*.vue'Length of output: 278
Script:
#!/bin/bash # Description: Re-verify styles that hide options without using 'display: none' # Test: Search for CSS rules that hide elements using 'visibility: hidden' or 'opacity: 0' rg 'visibility:\s*hidden|opacity:\s*0' --type css --glob '*.css' # Test: Search for inline styles in Vue component templates that hide elements rg 'style=.*(visibility:\s*hidden|opacity:\s*0)' --glob '*.vue'Length of output: 266
packages/renderless/src/select/index.ts (2)
Line range hint
1880-1890: Inefficient DOM Access inresetInputHeightIn the
resetInputHeightfunction, multiple DOM reads and writes occur without batching, which can lead to layout thrashing. Use a utility likefastdomto batch reads and writes for better performance.[performance]
Apply this diff to batch DOM operations:
export const resetInputHeight = ({ constants, nextTick, props, vm, state, api, designConfig }) => () => { if (state.collapseTags && !(props.filterable || props.searchable)) { return } nextTick(() => { - if (!vm.$refs.reference) { + const reference = vm.$refs.reference + if (!reference) { return } - let input = vm.$refs.reference.type === 'text' && vm.$refs.reference.$el.querySelector('input') - const tags = vm.$refs.tags + let input + let tags + fastdom.measure(() => { + input = reference.type === 'text' && reference.$el.querySelector('input') + tags = vm.$refs.tags // existing logic... + }) }) }
Line range hint
1200-1215: Inconsistent Usage ofdeepCloneFunctionThe
emitChangefunction usesdeepCloneonvaluebefore comparing it withstate.compareValue. Ensure thatdeepCloneis necessary here, and if so, consistently apply it wherever comparisons withstate.compareValueare made.Run the following script to check for consistent use of
deepClone:
| // 字号（loading、空数据等辅助提示类） | ||
| --tv-SelectDropdown-font-size-tip: var(--tv-font-size-sm, 12px); | ||
| // 文本色（loading、空数据等辅助提示类） | ||
| --tv-SelectDropdown-text-color-tip: var(---tv-color-text-secondary, #595959); |
There was a problem hiding this comment.
Fix typo in CSS variable name
In line 21, there is an extra hyphen in the variable name within the var() function. It should have two hyphens instead of three:
--tv-SelectDropdown-text-color-tip: var(--tv-color-text-secondary, #595959);| .checked-sur.tiny-svg-size, | ||
| .halfselect.tiny-svg-size { | ||
| fill: var(--ti-option-multi-svg-icon-color); | ||
| &:not(:has(.tiny-option__checkbox-wrap)).selected { |
There was a problem hiding this comment.
💡 Codebase verification
Compatibility Issues Found with :has() Pseudo-Class
The :has() pseudo-class is used in multiple locations within the codebase, which may lead to compatibility issues across different browsers. Consider refactoring these selectors to enhance cross-browser support.
Affected Files:
packages/theme/src/textarea/index.less:.@{textarea-prefix-cls}:has(&:focus) {packages/theme/src/input/index.less:&-append > div:has(> .@{input-prefix-cls}__suffix) {packages/theme/src/option/index.less:&:not(:has(.tiny-option__checkbox-wrap)).selected {packages/theme/src/nav-menu/index.less:&:has(.only-secondary-title) {packages/theme/src/float-button/index.less:&:has(.@{svg-prefix-cls}) {packages/theme/src/link/index.less:&:has(.@{svg-cls}):hover {packages/theme/src/grid/table.less:&:has(> .@{grid-prefix-cls}-checkbox) {packages/theme/src/form-item/index.less:&:has(> label.is-group-display-only) {packages/theme/src/dropdown-menu/index.less:&:has(.has-children) > li:not(.has-children) {packages/theme/src/date-table/index.less:&__row .disabled:not(:has(+ .disabled)) div{packages/theme-saas/src/grid/table-global.less:&__tooltip:has(.tiny-grid-tree__indent:nth-last-child(2):first-child),packages/theme-saas/src/tree/index.less:&:has(.is-disabled) {packages/theme-saas/src/scrollbar/index.less:&:has(.@{css-prefix}scrollbar__thumb:hover) {packages/theme-saas/src/query-builder-pro/index.less:.rule .rule-value:has(.rule-value-list-item) {packages/theme-saas/src/picker/index.less:&:has(.@{css-prefix}scrollbar__thumb:hover) {packages/theme-saas/src/collapse-item/index.less:&:has(.@{collapse-item-prefix-cls}__arrow:hover) {packages/theme-saas/src/amount/index.less:&:has(.is-display-only) {
🔗 Analysis chain
Avoid Using ':has()' Pseudo-Class for Compatibility
The :has() pseudo-class used in the selector &:not(:has(.tiny-option__checkbox-wrap)).selected (line 83) is not widely supported across all browsers, which may lead to compatibility issues for users on certain platforms.
Consider refactoring the selector to achieve the desired styling without relying on :has(). This could involve restructuring the HTML markup or using alternative CSS selectors that are more widely supported.
To identify all usages of the :has() pseudo-class in the codebase and assess potential compatibility risks, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of the ':has()' pseudo-class in all LESS files.
# Expected result: List all occurrences of ':has(' to identify where it is used.
rg --type less ':has\('
Length of output: 1174
Script:
#!/bin/bash
# Description: Search for usage of the ':has()' pseudo-class in all LESS files.
# Expected result: List all occurrences of ':has(' to identify where it is used.
rg --type less ":has\(“
Length of output: 93
Script:
#!/bin/bash
# Description: Search for usage of the ':has()' pseudo-class in all LESS files.
# Expected result: List all occurrences of ':has(' to identify where it is used.
rg --type less ':has\('
Length of output: 4055
| max-height: 300px; | ||
| overflow-y: auto; | ||
| .tiny-svg { | ||
| font-size: var(--tv-SelectDropdown-icon-size); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using width and height for SVG sizing
Using font-size on .tiny-svg may not affect inline SVG elements as expected. It might be more appropriate to use width and height properties to control SVG size for better compatibility.
| <template v-else> | ||
| <span v-if="showEmptyImage" class="tiny-select-dropdown__empty-images"></span> | ||
| <span v-else class="tiny-select-dropdown__empty"> {{ state.emptyText }}</span> | ||
| </template> |
There was a problem hiding this comment.
Correct the misuse of v-else with <template>
The v-else directive is improperly used on a <template> tag following a closing </div>:
</div>
<template v-else>
<span v-if="showEmptyImage" class="tiny-select-dropdown__empty-images"></span>
<span v-else class="tiny-select-dropdown__empty"> {{ state.emptyText }}</span>
</template>In Vue.js, v-else must be placed on an element immediately after the v-if or v-else-if element, and they must be siblings. Also, using v-else on a <template> is inappropriate here.
Here's how you can fix it:
</div>
-<template v-else>
+<div v-else class="tiny-select-dropdown__empty-wrap">
<span v-if="showEmptyImage" class="tiny-select-dropdown__empty-images"></span>
<span v-else class="tiny-select-dropdown__empty"> {{ state.emptyText }}</span>
-</template>
+</div>This ensures that the v-else is correctly applied to a single root element and follows Vue's syntax rules.
| <!-- tiny 新增 面板搜索 --> | ||
| <tiny-input | ||
| v-if="searchable" | ||
| input-box-type="underline" | ||
| v-model="state.query" | ||
| :placeholder="t('ui.search.placeholder')" |
There was a problem hiding this comment.
Correct the event handler for tiny-input
In the <tiny-input> component, you're using v-model="state.query" and also binding @update:modelValue. The handleQueryChange method is being called with state.query, which may not reflect the latest input value. Instead, the event handler receives the updated value as an argument. You should modify the event handler to accept this value.
Here's a suggested fix:
<tiny-input
v-if="searchable"
input-box-type="underline"
v-model="state.query"
:placeholder="t('ui.search.placeholder')"
class="tiny-select-dropdown__search"
- @update:modelValue="handleQueryChange(state.query)"
+ @update:modelValue="handleQueryChange"
>This way, handleQueryChange will receive the new input value directly.
Committable suggestion was skipped due to low confidence.
| @@ -1766,7 +1766,7 @@ const optmzApis = { | |||
| '.tiny-recycle-scroller__slot, .tiny-recycle-scroller__item-view:not([style*="transform: translateY(-9999px) translateX(0px)"])' | |||
| ) | |||
| ) | |||
| .map((item) => item.querySelector(`[data-tag="tiny-select-dropdown-item"]:not(${querySelectKey})`)) | |||
| .map((item) => item.querySelector(`[data-tag="tiny-option"]:not(${querySelectKey})`)) | |||
There was a problem hiding this comment.
Correct Method Chaining Syntax
At line 1769, there is a dangling line that seems to be incorrectly formatted:
.map((item) => item.querySelector(`[data-tag="tiny-option"]:not(${querySelectKey})`))Ensure that this line is properly connected to the preceding code and that the chaining syntax is correct.
| .map((item) => item.querySelector(`[data-tag="tiny-option"]:not(${querySelectKey})`)) | ||
| .filter((v) => v) | ||
| }, |
There was a problem hiding this comment.
Null Check Needed in queryVisibleOptions Method
In the queryVisibleOptions function, there is a potential for item to be null or undefined, which would cause querySelector to throw an error. It's advisable to add a null check before attempting to call querySelector.
Apply this diff to add a null check:
.map((item) =>
- item.querySelector(`[data-tag="tiny-option"]:not(${querySelectKey})`)
+ item && item.querySelector(`[data-tag="tiny-option"]:not(${querySelectKey})`)
)📝 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.
| .map((item) => item.querySelector(`[data-tag="tiny-option"]:not(${querySelectKey})`)) | |
| .filter((v) => v) | |
| }, | |
| .map((item) => item && item.querySelector(`[data-tag="tiny-option"]:not(${querySelectKey})`)) | |
| .filter((v) => v) | |
| }, |
…actor styles for selectDropdown
…" to data-tag="tiny-option"
…or styles for option-group
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
base-selectandselectcomponents with new methods for improved search and selection management.Bug Fixes
Refactor
Style