refactor(dropdown-menu): [dropdown] refactor DropdownMenu and DropdownItem vars and style #2208
Conversation
WalkthroughThe changes in this pull request involve updates to the dropdown component's styling, functionality, and associated tests. Key modifications include adjustments to CSS variables for theming, enhancements in the state management of dropdown items, and refinements in the rendering logic. The updates aim to streamline the styling approach and improve the overall flexibility of the dropdown components. Changes
Possibly related PRs
Suggested labels
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
MomoPoppy
changed the title
MomoPoppy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
examples/sites/demos/pc/app/dropdown/visible-arrow.spec.ts (1)
20-20: LGTM. Consider adding a comment for clarity.The change from
-6pxto-8pxfor the arrow's top CSS property looks good. This adjustment likely improves the visual alignment of the dropdown arrow.Consider adding a brief comment explaining the reason for this change. For example:
// Adjusted arrow position for better visual alignment await expect(arrow).toHaveCSS('top', '-8px')This would help future developers understand the intent behind this specific value.
packages/theme/src/dropdown-menu/vars.less (2)
17-17: Good use of semantic color variable with fallback.The
--tv-DropdownMenu-bg-colorvariable effectively uses a semantic color variable with a fallback, which is a best practice for theming and maintainability.Consider adding a comment explaining the significance of
--tv-color-bg-secondaryin the context of the DropdownMenu, if it's not obvious from the variable name alone.
15-25: Overall improvement in variable structure and naming convention.The refactoring of CSS variables for the DropdownMenu component significantly enhances consistency and maintainability. The new
--tv-prefix and component-based naming convention improve code readability. Leveraging design system variables for colors, shadows, and spacing ensures consistency across the application and facilitates easier global updates.However, there are opportunities for further improvement:
- Consider using relative units for fixed pixel values to enhance responsiveness.
- Evaluate the combination of vertical and horizontal padding into a single variable for potential separation.
- Ensure that the removal of specific variables (e.g., border-related variables) doesn't unintentionally affect the component's styling.
To maintain the integrity of these changes across the entire component, ensure that all related files (JavaScript/TypeScript, Vue templates) are updated to use these new CSS variables consistently.
packages/vue/src/dropdown-item/src/pc.vue (1)
17-25: Improved class binding structure.The refactoring of the class binding from object syntax to array syntax enhances readability and maintainability. The addition of
state.sizeClassprovides better flexibility for component sizing.Consider using template literals for multi-line class bindings to improve readability further:
- :class="[ - 'tiny-dropdown-item', - 'tiny-dropdown-menu__item', - state.sizeClass, - disabled ? 'is-disabled' : '', - divided ? 'tiny-dropdown-item--divided tiny-dropdown-menu__item--divided' : '', - state.checkedStatus && selected ? 'tiny-dropdown-item--check-status' : '', - itemData.children && itemData.children.length ? 'has-children' : '' - ]" + :class="` + tiny-dropdown-item + tiny-dropdown-menu__item + ${state.sizeClass} + ${disabled ? 'is-disabled' : ''} + ${divided ? 'tiny-dropdown-item--divided tiny-dropdown-menu__item--divided' : ''} + ${state.checkedStatus && selected ? 'tiny-dropdown-item--check-status' : ''} + ${itemData.children && itemData.children.length ? 'has-children' : ''} + `"This approach can make the class binding even more readable, especially for longer lists of classes.
packages/theme/src/dropdown-item/vars.less (1)
15-61: Consider unifying the language of code commentsThe code comments within this file are in Chinese, while the header comments are in English. For consistency and to facilitate collaboration in an international team, it might be beneficial to use a single language for all comments, preferably English.
packages/theme/src/dropdown-menu/index.less (2)
32-32: Avoid unnecessary nesting of the same class selectorThe nested selector
.@{dropdown-menu-prefix-cls}within itself may be unnecessary and could increase CSS specificity unnecessarily. Consider flattening the structure to improve maintainability.
35-35: Translate code comments into English for consistencySeveral comments are written in Chinese at lines 35, 42, and 49. To ensure consistency and readability for all team members, please translate code comments into English.
Also applies to: 42-42, 49-49
packages/theme/src/dropdown-item/index.less (2)
49-50: Remove duplicatecontentproperty in:beforepseudo-elementThere are two identical
content: '';declarations in the:beforepseudo-element. The second declaration overrides the first, making the first redundant.Consider removing the duplicate line:
&:before { - content: ''; content: ''; height: 0px; display: block; margin: 0px; }
114-114: Translate code comments to English for consistencyThere are comments in Chinese on lines 114 (
// 展开项悬浮激活态), 135 (// 选中态), and 141 (// 分割线). To ensure consistency and readability for all contributors, please consider translating these comments into English.Also applies to: 135-135, 141-141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- examples/sites/demos/pc/app/dropdown/visible-arrow.spec.ts (1 hunks)
- packages/renderless/src/dropdown-item/vue.ts (3 hunks)
- packages/theme/src/dropdown-item/index.less (4 hunks)
- packages/theme/src/dropdown-item/vars.less (1 hunks)
- packages/theme/src/dropdown-menu/index.less (3 hunks)
- packages/theme/src/dropdown-menu/vars.less (1 hunks)
- packages/vue/src/dropdown-item/src/pc.vue (2 hunks)
🔇 Additional comments (25)
packages/theme/src/dropdown-menu/vars.less (4)
15-15: Improved naming convention and reusability.The introduction of
--tv-DropdownMenu-box-shadowusing the--tv-shadow-3-downvariable is a good practice. It enhances consistency across the component and improves maintainability by leveraging predefined shadow styles.
19-19: Consistent use of design system variables.The
--tv-DropdownMenu-border-radiusvariable effectively leverages the design system's border radius variable. This approach ensures consistency across components and facilitates easier global style updates.
23-23: Effective use of design system spacing variable.The
--tv-DropdownMenu-margin-yvariable appropriately uses a design system spacing variable. This approach ensures consistent spacing across components and aligns with best practices for maintainable CSS.
24-25: 🛠️ Refactor suggestionConsider separating vertical and horizontal padding.
The
--tv-DropdownMenu-paddingvariable combines vertical and horizontal padding, which might limit flexibility in some scenarios.
- Consider separating vertical and horizontal padding for more granular control:
--tv-DropdownMenu-padding-y: var(--tv-space-md, 8px); --tv-DropdownMenu-padding-x: 0px;
- Confirm if 0px horizontal padding is intentional, as it might affect the appearance of menu items.
To ensure this change doesn't negatively impact the layout, please run the following command to check for any hardcoded padding values in the DropdownMenu component:
✅ Verification successful
Padding with 0px Confirmed as Intentional
No additional hardcoded padding values with
0pxwere found in theDropdownMenucomponent. The use of0pxfor horizontal padding in--tv-DropdownMenu-paddingappears intentional.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type less --type vue 'padding:.*?0(px)?' packages/theme/src/dropdown-menu packages/vue/src/dropdown-menuLength of output: 139
Script:
#!/bin/bash # Searching for padding patterns with 0px in .less and .vue files rg 'padding:.*?0(px)?' packages/theme/src/dropdown-menu packages/vue/src/dropdown-menu -g '*.less' -g '*.vue'Length of output: 212
packages/vue/src/dropdown-item/src/pc.vue (1)
Line range hint
1-114: Summary of changesThe refactoring of the DropdownItem component has successfully improved code readability and maintainability without introducing functional changes. The modifications, particularly in the class binding structure, align with modern Vue.js best practices.
Key points:
- Class binding refactored from object to array syntax, enhancing readability.
- Addition of
state.sizeClassfor improved flexibility in component sizing.- A TODO comment added to evaluate the necessity of the 'tiny-svg-size' class.
These changes meet the PR objectives of refactoring without altering functionality or introducing breaking changes. The code quality has been improved, making future maintenance and updates easier.
packages/renderless/src/dropdown-item/vue.ts (2)
61-61: LGTM: Function signature update is appropriate.The addition of the
dropdownVmparameter to theinitStatefunction signature is consistent with the refactoring objectives. This change allows the function to access properties from thedropdownVmobject, which is used for size-related styling.
Line range hint
1-144: Overall assessment: Well-executed refactoring with minor suggestions for improvement.The changes made to the
packages/renderless/src/dropdown-item/vue.tsfile are consistent with the PR objectives of refactoring the DropdownMenu and DropdownItem components. The modifications enhance the flexibility of the dropdown item by introducing size-based styling and improve the overall structure of the code.Key points:
- The
initStatefunction now accepts adropdownVmparameter, allowing for more contextual styling.- A new
sizeClassproperty has been added to the state, enabling dynamic size-based styling.- The
renderlessfunction now injects thedropdownVm, integrating it into the component's lifecycle.These changes appear to be non-breaking and improve the maintainability of the code. The minor suggestions provided in the review comments could further enhance readability and type safety.
packages/theme/src/dropdown-item/vars.less (2)
14-61: Refactoring improves variable naming consistencyThe updated variable names with the
--tv-prefix enhance the clarity and consistency of the codebase, aligning with the new naming conventions.
14-61: Verify variable renaming is consistent throughout the codebaseWhile the variables have been updated from the
--ti-prefix to--tv-in this file, please ensure that all references to these variables in other files have been updated accordingly to prevent any broken styles or unexpected behavior.You can run the following script to check for any remaining usages of the old variable names:
packages/theme/src/dropdown-menu/index.less (2)
23-29: Consistent use of CSS variables for dropdown menu stylingThe updated CSS variables ensure consistent theming throughout the component.
36-36:⚠️ Potential issuePotential browser compatibility issues with the
:has()CSS pseudo-classThe
:has()pseudo-class is not fully supported in all browsers, notably lacking support in Firefox. This may cause inconsistent behavior in browsers that do not support it.Consider using alternative selectors or implementing a polyfill to ensure compatibility with all target browsers.
Also applies to: 43-43
packages/theme/src/dropdown-item/index.less (14)
23-26: Appropriate use of theming variables for typographyThe font size, height, line-height, and text color are updated to use the new theming variables, enhancing consistency and maintainability.
31-33: Improved layout with flexboxApplying
display: flexandalign-items: centerimproves the layout and alignment of the dropdown items.
36-36: Enhancement of spacing for dropdown itemsThe addition of padding and the adjustment of margin enhance the spacing and visual appeal of the dropdown items.
Also applies to: 39-39
43-46: Consistent SVG icon styling using theming variablesApplying theming variables to the SVG icon's font size and fill color ensures consistent styling across components.
59-60: Setting wrapper dimensions to full sizeSpecifying
height: 100%andwidth: 100%for the wrapper ensures it occupies the full available space, which is suitable for alignment purposes.
72-72: Adjusting margin for expand iconThe updated
margin-rightfor the expand icon improves spacing and layout.
90-91: Adding margin to pre-icon for better spacingThe margin adjustment on the pre-icon enhances the spacing between the icon and the label.
98-98: Consistent hover background colorUpdating the background color on hover to use the theming variable ensures consistency across components.
104-104: Updating icon color on hoverChanging the SVG icon's fill color on hover to use the theming variable maintains visual consistency.
116-117: Improving active state styling for items with childrenThe updates to
colorandbackground-colorenhance the visual indication when hovering over items with children.
123-123: Applying disabled text colorUsing the theming variable for the disabled text color ensures consistency in styling disabled items.
129-129: Setting icon color for disabled stateUpdating the fill color of SVG icons in the disabled state maintains visual consistency.
135-139: Styling for checked stateThe addition of styles for the checked state (
&--check-status) using theming variables appropriately reflects the active selection.
150-156: Adding size modifiers for small and mini variantsIntroducing
&--smalland&--minimodifiers with appropriate heights enhances the component's versatility.
| // 下拉菜单的最小宽度 | ||
| --ti-dropdown-menu-min-width: 124px; | ||
| --tv-DropdownMenu-min-width: 124px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a relative unit for minimum width.
While the --tv-DropdownMenu-min-width variable follows the new naming convention, using a fixed pixel value (124px) might limit flexibility in responsive designs.
Consider using a relative unit like rem or em, or referencing a design system spacing variable for better scalability across different screen sizes and font settings. For example:
--tv-DropdownMenu-min-width: 7.75rem; /* Equivalent to 124px at 16px base font size */| <!-- TODO: 评估没什么用的话去掉 tiny-svg-size --> | ||
| <component v-if="icon" :is="icon" class="tiny-svg-size tiny-dropdown-item__pre-icon"></component> |
There was a problem hiding this comment.
💡 Codebase verification
Evaluate the impact before removing the 'tiny-svg-size' class.
The tiny-svg-size class is used extensively across multiple components, as identified by the shell script results. Removing it could significantly affect the styling and functionality of various UI elements.
To manage this properly, consider creating a dedicated issue to thoroughly assess the implications of removing or modifying the tiny-svg-size class. This will ensure that all dependencies are evaluated and necessary adjustments are made without disrupting the current functionality.
🔗 Analysis chain
Clarify the TODO comment and consider creating an issue.
The TODO comment suggests evaluating the removal of the 'tiny-svg-size' class, which is a good step towards code optimization. However, more context would be helpful.
Could you provide more information about why this class might be unnecessary? What impact would its removal have on the component's functionality or styling?
To ensure this task isn't forgotten, consider creating a separate issue to track this evaluation. This will allow for a more thorough investigation without blocking the current PR.
This script will help identify other usages of the 'tiny-svg-size' class and its definition, which can aid in the evaluation process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other occurrences of 'tiny-svg-size' class in the codebase
# Test: Search for 'tiny-svg-size' class usage
echo "Occurrences of 'tiny-svg-size' class in the codebase:"
rg --type vue --type css "tiny-svg-size"
# Test: Check if 'tiny-svg-size' is defined in any CSS/LESS files
echo "\nDefinition of 'tiny-svg-size' class in CSS/LESS files:"
rg --type css "\.tiny-svg-size"
Length of output: 354
Script:
#!/bin/bash
# Description: Correctly search for occurrences and definitions of 'tiny-svg-size' class in the codebase
# Search for 'tiny-svg-size' class usage in .vue and .css/.less files
echo "Occurrences of 'tiny-svg-size' class in .vue and .css/.less files:"
rg "tiny-svg-size" --glob "*.vue" --glob "*.css" --glob "*.less" -A 2
# Search for the definition of 'tiny-svg-size' class in .css/.less files
echo "\nDefinition of 'tiny-svg-size' class in CSS/LESS files:"
rg "\.tiny-svg-size" --glob "*.css" --glob "*.less"
Length of output: 55403
| const dropdownVm = inject('dropdownVm', null) | ||
| const state: IDropdownItemState = initState({ reactive, computed, api, props, parent, dropdownMenuVm, dropdownVm }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Injection of dropdownVm is consistent with other changes.
The injection of dropdownVm and its usage in the initState function call are consistent with the changes made to the initState function signature. This change properly integrates the new dropdownVm parameter into the component's lifecycle.
For consistency with the dropdownMenuVm injection, consider adding a type annotation for dropdownVm:
- const dropdownVm = inject('dropdownVm', null)
+ const dropdownVm = inject('dropdownVm', null) as IDropdownVm | nullThis change would improve type safety and make the code more consistent with the existing style.
📝 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.
| const dropdownVm = inject('dropdownVm', null) | |
| const state: IDropdownItemState = initState({ reactive, computed, api, props, parent, dropdownMenuVm, dropdownVm }) | |
| const dropdownVm = inject('dropdownVm', null) as IDropdownVm | null | |
| const state: IDropdownItemState = initState({ reactive, computed, api, props, parent, dropdownMenuVm, dropdownVm }) |
| @@ -78,6 +78,7 @@ | |||
| currentIndex: props.currentIndex, | |||
| textField: dropdownMenuVm?.textField || props.textField, | |||
| popperClass: dropdownMenuVm?.popperClass || '', | |||
| sizeClass: dropdownVm?.size ? `tiny-dropdown-item--${dropdownVm?.size}` : '', | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: New sizeClass property enhances flexibility.
The addition of the sizeClass property allows for dynamic size-based styling of the dropdown item, which is consistent with the refactoring objectives. This change enhances the component's flexibility.
Consider extracting the size class generation into a separate computed property for improved readability:
- sizeClass: dropdownVm?.size ? `tiny-dropdown-item--${dropdownVm?.size}` : '',
+ sizeClass: computed(() => dropdownVm?.size ? `tiny-dropdown-item--${dropdownVm.size}` : ''),This change would make the code more declarative and easier to maintain.
Committable suggestion was skipped due to low confidence.
| --tv-DropdownItem-height-sm: var(--tv-size-height-sm, 28px); | ||
| // 菜单项高度(mini） | ||
| --tv-DropdownItem-height-xs: var(--tv-size-height-mi, 24px); |
There was a problem hiding this comment.
Potential inconsistency in size variable naming
It appears that in line 56, --tv-DropdownItem-height-xs is set using --tv-size-height-mi, whereas the naming convention suggests it should be --tv-size-height-xs. This could lead to confusion and inconsistency with standard size naming conventions (xs, sm, md, etc.). Please verify if --tv-size-height-mi should be --tv-size-height-xs to align with the variable name.
Suggested change:
--tv-DropdownItem-height-xs: var(--tv-size-height-mi, 24px);
+--tv-DropdownItem-height-xs: var(--tv-size-height-xs, 24px);📝 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-DropdownItem-height-sm: var(--tv-size-height-sm, 28px); | |
| // 菜单项高度(mini） | |
| --tv-DropdownItem-height-xs: var(--tv-size-height-mi, 24px); | |
| --tv-DropdownItem-height-sm: var(--tv-size-height-sm, 28px); | |
| // 菜单项高度(mini） | |
| --tv-DropdownItem-height-xs: var(--tv-size-height-xs, 24px); |
| border-bottom-width: 0; | ||
| } | ||
|
|
||
| &.@{css-prefix}popper[x-placement^='top'] .popper__arrow:after { | ||
| bottom: 1px; | ||
| margin-left: -6px; | ||
| border-top-color: var(--ti-dropdown-menu-arrow-after-border-color); | ||
| border-top-color: #fff; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use CSS variables instead of hardcoded color values for consistency
The use of hardcoded color values like #fff reduces flexibility in theming and maintenance. Consider replacing them with existing CSS variables or defining new ones to maintain consistency across the component.
Apply these changes:
At line 87:
- border-top-color: #fff;
+ border-top-color: var(--tv-DropdownMenu-arrow-color);At line 108:
- border-bottom-color: #fff;
+ border-bottom-color: var(--tv-DropdownMenu-arrow-color);At line 125:
- border-right-color: #fff;
+ border-right-color: var(--tv-DropdownMenu-arrow-color);At line 145:
- border-left-color: #fff;
+ border-left-color: var(--tv-DropdownMenu-arrow-color);Also applies to: 108-108, 125-125, 145-145
…actor styles for dropdowm-menu
…actor styles for dropdowm-item
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
Refactor