refactor(theme): [nav-menu] refactor nav-menu theme vars #2256
Conversation
WalkthroughThe pull request introduces several modifications to Vue components and CSS files related to the navigation menu. Key changes include updates to the styling of icons within components, a restructuring of CSS variable names to a new naming convention, and enhancements to the layout and presentation of the navigation menu. The overall functionality and structure of the components remain unchanged, with a focus on improving visual consistency and readability in the CSS. Changes
Possibly related PRs
Suggested labels
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: 4
🧹 Outside diff range and nitpick comments (2)
examples/sites/demos/pc/app/nav-menu/slot-toolbar.vue (1)
332-335: Approve style changes with a suggestion.The style changes for the icons look good. They improve the visual presentation of the navigation menu toolbar.
Consider using CSS custom properties (variables) for the color and size values to improve maintainability:
:root { --icon-color: #808080; --icon-size: 24px; --icon-margin: 20px; } .preview-nav .icon-setting, .preview-nav .icon-administrator { margin-right: var(--icon-margin); fill: var(--icon-color); font-size: var(--icon-size); }This approach would make it easier to update these values consistently across the component if needed in the future.
packages/theme/src/nav-menu/index.less (1)
31-31: Consider using variables for margin valuesThe margin is set with hardcoded values
margin: 0 48px 0 16px;. Using CSS variables for these values can improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- examples/sites/demos/pc/app/nav-menu/slot-toolbar-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/nav-menu/slot-toolbar.vue (1 hunks)
- packages/theme/src/nav-menu/index.less (12 hunks)
- packages/theme/src/nav-menu/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
examples/sites/demos/pc/app/nav-menu/slot-toolbar-composition-api.vue (1)
325-328: Verify the design changes and consider accessibilityThe changes to the icon styling look good in terms of code organization. Combining the selectors for
.icon-settingand.icon-administratorreduces code duplication. However, please consider the following points:
The
fillcolor has been changed fromwhiteto#808080(a medium gray). Ensure this change aligns with the overall design and maintains sufficient contrast with the background for accessibility.A
font-sizeof 24px has been added. Verify that this size is consistent with other icons in the application and meets any design system requirements.To ensure these changes align with the design system and maintain accessibility, please run the following checks:
examples/sites/demos/pc/app/nav-menu/slot-toolbar.vue (2)
Line range hint
1-335: Summary: Changes align with PR objectives.The modifications in this file successfully contribute to the refactoring of nav-menu theme variables:
- Updated component imports from '@opentiny/vue' and '@opentiny/vue-icon'.
- Improved icon styling in the navigation menu toolbar.
These changes enhance the visual consistency and maintainability of the navigation menu component without altering its functionality, which aligns well with the PR's refactoring objectives.
Line range hint
17-19: Verify icon component instantiation.The icon components are being instantiated as functions (
IconSetting()andIconAdministrator()). This is an uncommon pattern for component registration. Please verify if this is the intended usage according to the@opentiny/vue-icondocumentation.If not intended, consider changing to standard component registration:
components: { TinyNavMenu: NavMenu, IconSetting, IconAdministrator }To confirm the correct usage, please run the following command to check the documentation or examples:
This will search for other usages of these icon components in the project and show how they are typically registered.
packages/theme/src/nav-menu/vars.less (1)
13-13: Ensure All References to the Old Function Name Are UpdatedThe function
.component-css-vars-nav-menu()has been renamed to.inject-NavMenu-vars(). Please verify that all instances of the old function name have been updated throughout the codebase to prevent any potential issues.Run the following script to identify any remaining references to the old function name:
✅ Verification successful
All references to the old function name have been successfully updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of `.component-css-vars-nav-menu()` in the codebase. rg --type less --type css $'\.component-css-vars-nav-menu\(\)'Length of output: 63
Script:
#!/bin/bash # Description: Search for all occurrences of `.component-css-vars-nav-menu()` in the codebase, allowing for optional whitespace. rg '\.component-css-vars-nav-menu\s*\(' --type less --type cssLength of output: 64
packages/theme/src/nav-menu/index.less (2)
Line range hint
211-211: Inconsistent variable prefix:--ti-common-font-size-baseThis line also references
--ti-common-font-size-base. To maintain consistency with thetvprefix, consider updating it.
20-20: Verify the definition of.inject-NavMenu-vars()mixinThe mixin
.inject-NavMenu-vars()is used to inject variables. Please ensure that it is defined and imported correctly in your codebase.Run the following script to confirm the mixin's existence:
| --tv-NavMenu-popmenu-node-title-font-size: var(--tv-font-size-md); | ||
| // 导航菜单下拉框标题字号 | ||
| --ti-nav-menu-popmenu-text-font-size: var(--ti-common-font-size-0, 12px); | ||
| --tv-NavMenu-popmenu-text-font-size: var(--tv-font-size-sm); |
There was a problem hiding this comment.
Correct Duplicate Comments for Font Size Variables
The comments for lines 61 and 63 are identical:
// 导航菜单下拉框标题字号However, they correspond to different variables:
- Line 61:
--tv-NavMenu-popmenu-node-title-font-size - Line 63:
--tv-NavMenu-popmenu-text-font-size
Please update the comment on line 63 to accurately describe the purpose of --tv-NavMenu-popmenu-text-font-size.
| background: var(--ti-nav-menu-item-selected-underline-bg-color); | ||
| border: 1px solid var(--ti-nav-menu-item-selected-underline-bg-color); | ||
| border-radius: var(--ti-nav-menu-item-selected-border-radius); | ||
| height: 2px; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use variables for fixed dimensions
Fixed pixel values are used for height and width:
- Line 75:
height: 2px; - Line 113:
width: var(--tv-NavMenu-item-selected-underline-width);(Ensure this variable is consistently used) - Line 176:
width: var(--tv-NavMenu-popmenu-more-item-before-width);(Butheight: 12px;)
Consider defining variables for these dimensions to allow easier adjustments and theming.
Example for line 75:
-height: 2px;
+height: var(--tv-NavMenu-underline-height);Define --tv-NavMenu-underline-height in your variables.
Also applies to: 113-113, 176-176
| height: var(--ti-nav-menu-popmenu-more-item-height); | ||
| line-height: var(--ti-nav-menu-popmenu-more-item-height); | ||
| height: var(--tv-NavMenu-popmenu-more-item-height); | ||
| line-height: var(--tv-NavMenu-popmenu-more-item-height); | ||
| font-size: var(--ti-common-font-size-base); |
There was a problem hiding this comment.
Inconsistent variable prefix: --ti-common-font-size-base
The variable --ti-common-font-size-base uses the ti prefix, whereas other variables use the tv prefix. For consistency, consider updating it to --tv-common-font-size-base.
Apply this diff to align the prefix:
-font-size: var(--ti-common-font-size-base);
+font-size: var(--tv-common-font-size-base);📝 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.
| font-size: var(--ti-common-font-size-base); | |
| font-size: var(--tv-common-font-size-base); |
| content: ''; | ||
| width: var(--ti-nav-menu-popmenu-more-item-before-width); | ||
| width: var(--tv-NavMenu-popmenu-more-item-before-width); | ||
| height: 12px; | ||
| background: #5167f1; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded color #5167f1 with a CSS variable
The hardcoded color #5167f1 is used for the background property. To enhance theming capabilities, replace it with a CSS variable.
Apply this diff:
-background: #5167f1;
+background: var(--tv-NavMenu-popmenu-more-item-before-bg-color);Ensure that --tv-NavMenu-popmenu-more-item-before-bg-color is defined in your variables.
📝 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.
| background: #5167f1; | |
| background: var(--tv-NavMenu-popmenu-more-item-before-bg-color); |
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
TinyNavMenucomponent for improved navigation.