-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Adjust menu and node switching styles #8206
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
||
.dropdown-item:hover { | ||
background: var(--el-menu-item-bg-color-active); | ||
} | ||
</style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are potential improvements and potential issues identified:
Potential Improvements:
- The HTML structure is not consistent, with some elements using div's and others components.
- The use of v-else to handle different collapse states might need further optimization.
- Using the component slot feature makes it easier to customize the appearance of the dropdown button.
Potential Issues:
- The logic used to update globalStore.currentNode seems complex and can be simplified.
- The taskCount variable should be declared as an immutable value since it doesn't get updated inside the script section. This can also reduce performance overhead on updates.
- Error handling in the checkTask function could potentially cause a lot more re-renders than needed due to updating multiple reactive properties at once.
General Recommendations:
- Consider replacing the deprecated Vue 2 directive
@click
with its equivalent shorthand@click
. - Use CSS Grid or Flexbox instead of nested absolute positioning where possible for better layout control and maintenance.
- Make sure there are unit tests for new functionality to prevent regressions and to ensure reliability over time.
- Avoid mixing template styles like SCSS directly into templates unless absolutely necessary - consider breaking off common styles into separate files.
font-weight: 500; | ||
color: var(--el-color-primary); | ||
height: 38px; | ||
} | ||
.ico { | ||
height: 20px !important; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provided changes appear to be making adjustments to the navigation bar component in an application. Here's a review of the key points:
Positive Changes:
-
Improved Button Positioning: The alignment and spacing have been improved around the dropdown button, reducing some overlap between elements.
-
Removed Unnecessary Code: The nodes management-related functions (
loadNodes
,changeNode
, etc.) were removed from the template and corresponding methods, which is likely more efficient for performance. -
Optimized Task Display: Removed duplicate method calls that checked for executing tasks (
checkTask
andopenTask
). Instead, only one is used, but it seems there might not actually be any issue with this approach as they don't significantly alter behavior. -
Code Consistency: Various instances where similar logic was repeated across different places within the codebase were corrected and unified.
Negative Changes:
-
Element Class Conflict: The
.background
class no longer exists, potentially affecting styling unless another class replaced it elsewhere in the project. -
Dropdown Button Hover Effect Missing: The tooltip text ("Load Node") does not display when hovering over the dropdown button, requiring additional CSS or UI improvements.
-
Functionality Removal: The
logout
function has been commented out, implying potential functionality loss without documentation about what specific action should replace it if needed.
Given these observations, here’s an updated checklist based on the reviewed aspects:
Summary Checklist
-
Styling Improvements: Ensure consistent and visually appealing layout.
-
Button Placement & Alignment: Verify correct position and padding for the dropdown button.
-
Unused Function Calls: Double-check why multiple task checks are present; comment them out or refactor based on necessary usage.
-
Missing Tooltip Text: Re-introduce tooltip logic for the dropdown button.
-
UI Feedback on Logout Actions: Consider adding appropriate feedback after calling the logout API.
Ensure thorough testing post-deployment to address any newly introduced bugs caused directly or indirectly due to code changes.
@@ -163,6 +185,9 @@ onMounted(() => { | |||
bottom: 0; | |||
left: 0; | |||
overflow: hidden; | |||
.affix { | |||
z-index: 5; | |||
} | |||
} | |||
|
|||
.hideSidebar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code has several changes:
-
The
app-sidebar
div now includes anel-affix
component with a custom offset and icon button for collapsing the sidebar on mouseenter/leave events. -
A new variable
collapseButtonShow
is introduced to control the visibility of the toggle button inside the affix. -
Event listeners have been added to handle clicks outside the sidebar (
@clickOutside
) and toggling the sidebar's collapse state (@mouseenter
,@mouseleave
). -
The
watch
function now listens to theisLoading
property globally from the store, which controls the loading status throughout the application. When it becomesfalse
, it triggers a call to reset some variables like collapsing states or other UI adjustments. -
Some minor styling tweaks were made to add a higher z-index for elements such as the collapsible menu button in the affix.
Here are optimization suggestions and potential issues noted:
Optimization Suggestions:
-
Efficient State Management: Instead of using separate boolean flags (
mobile
,openSidebar
, etc.), consider using computed properties that combine these values dynamically based on the current user interface state. -
Avoid Redundant Computations: Ensure that no unnecessary computations occur when
taskListRef
is updated, especially if there large lists being manipulated by it. -
Reduce DOM Manipulation: Limit the number of times DOM elements are directly modified within event handlers or reactive updates. Use templates to define HTML structures instead where possible.
Potential Issues:
-
Complexity Increase: Adding extra logic can make components more complex, potentially leading to maintenance challenges over time.
-
Performance Degradation: Excessive computation or redundant rendering could impact performance. Regular profiling might be helpful to identify bottlenecks.
-
Consistency Across Devices: Ensure that behavior and visual effects across different devices (responsive design) remain consistent and expected by users.
If you provide specific areas needing further attention or additional context about the project, I can offer further guidance tailored to optimizing and debugging the code.
2694917
to
526ac5f
Compare
|
||
.dropdown-item:hover { | ||
background: var(--el-menu-item-bg-color-active); | ||
} | ||
</style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several improvements and corrections to make this code snippet:
Improvements:
- Component Replacements: Replace
Expand
andFold
with corresponding Vue components (ArrowDownward
andArrowUpward
) for better readability and functionality. - Code Structure: Organize related properties, methods, and event listeners together under appropriate headings like "Computed Properties", "Data Variables", "Methods", and "Event Listeners".
- Style Cleanup: Remove unnecessary whitespace for cleaner code formatting.
- Consistent Naming Conventions: Ensure consistent naming conventions between HTML directives and data variables.
Corrections:
- Vue Import Issues: Add necessary imports at the beginning of the file to ensure all used Vue components and utilities are correctly referenced.
Suggested Code (Updated):
<template>
<div>
<el-popover
placement="right"
:show-arrow="false"
:offset="0"
:width="200"
trigger="hover"
popper-class="custom-popover-dropdown"
>
<template #reference>
<div class="el-dropdown-link" v-if="!menuStore.isCollapse">
<el-badge is-dot :value="taskCount" :show-zero="false" :offset="[5, 5]">
<el-button link @click="openChangeNode" @mouseenter="openChangeNode">
<SvgIcon class="icon" iconName="p-gerenzhongxin1" />
{{ loadCurrentName() }}
</el-button>
</el-badge>
</div>
<div v-else class="el-dropdown-link">
<el-badge is-dot :value="taskCount" :show-zero="false" :offset="[-5, 5]">
<SvgIcon class="icon" iconName="p-gerenzhongxin1" />
</el-badge>
</div>
</template>
<div class="dropdown-menu">
<div class="dropdown-item mb-2" @click="openTask">
{{ $t('menu.msgCenter') }}
<el-tag class="msg-tag" v-if="taskCount !== 0" size="small" round>{{ taskCount }}</el-tag>
</div>
<el-divider v-if="nodes.length > 0" class="divider" />
<div v-if="nodes.length > 0" class="mt-2 mb-2">
<div class="dropdown-item" @click="changeNode('local')">
{{ $t('xpack.node.master') }}
</div>
<div
class="dropdown-item"
@click="changeNode(item.name)"
:disabled="item.status !== 'Healthy'"
v-for="item in nodes"
:key="item.name"
:style="{ color: item.status === 'Healthy' ? '' : '#FF6666' }"
>
{{ item.name }}
<el-icon class="icon-status" v-if="item.status !== 'Healthy'">
<Warning />
</el-icon>
</div>
</div>
<el-divider class="divider" />
<div class="dropdown-item mt-2" @click="logout">{{ $t('commons.login.logout') }}</div>
</div>
</el-popover>
</div>
</template>
<script setup lang='ts'>
import SvgIcon from '@/components/SvgIcon/index.vue'; // Assuming SvgIcon component exists in src/components directory
import Warning from '@element-plus/icons-vue/Warning';
import { ref, onMounted } from 'vue';
import { MenuStore, LogUtil, MessageUtils } from '@/store'; // Adjusted import paths based on actual module names
import EventBus from '@/global/bus';
import AuthAPI from '@/api/modules/auth';
const menuStore = new MenuStore(); // Create instance of MenuStore if it's not already imported globally
// Other setup logic...
Please apply these changes according to your project structure and requirements. This revision should improve maintainability and potentially eliminate any issues identified during testing.
font-weight: 500; | ||
color: var(--el-color-primary); | ||
height: 38px; | ||
} | ||
.ico { | ||
height: 20px !important; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Differences Between the Two Versions:
1. @ref
: Element Plus Ref Type Annotations
Old Version (v1):
import { DropdownInstance } from 'element-plus';
New Version (v2):
import { DropdownInstance } from 'element-plus/lib/components/dropdown'; // Adjust package path here
Recommendation:
Update the import statement to align with the correct path used in your project.
Old Code:
nodeChangeRef: ref<DropdownInstance>()
New Code:
nodeChangeRef: ref<DropdownInstance>() // Ensure consistent reference typing
2. getCheckedLabels
Function Signature Change
Old Version (v1):
function getCheckedLabels(
menu: Array<any> | undefined,
showMap: Map<string, boolean>
) {
...
}
New Version (v2):
function getCheckedLabels(
menu: readonly any[],
showMap?: Record<string, boolean>,
...children: unknown[]
): number {}
Recommendation:
Update the types for clarity.
3. Typing Consistency and Default Parameters
Old Version (v1):
let routerMenus = computed<>((): RouteRecordRaw[] => {
return menuStore.menuList.filter(...args) as RouteRecordRaw[];
});
New Version (v2):
const routerMenus = computed<
(...args: string[]) => RouteRecordRaw[]
>(() => menuStore.menuList.filter((...args) => true)); // Placeholder condition
Recommendation:
Use default parameter values when applicable.
4. Arrow Functions Syntax Changes
Old Version (v1):
handleMenuClick(path) {
this.$emit('menuClick', path);
}
logout() {
ElMessageBox({
...
}).then(() => {}).catch(() => {});
}
loadNodes() {
loadNodeOptions().then(res).catch(() => {});
}
changeNode(command: string) {
...
}
New Version (v2):
handleMenuClick(path: string): void {
this.emit('menuClick', path);
}
async logout(): Promise<void> {
const result = await ElMessageBox.confirm(i18n.global.t('commons.msg.sureLogOut'), i18n.global.t('commons.msg.infoTitle'), {
...
});
if (result === ElMessageBox.ConfirmResultEnum.CONFIRM) {
globalStore.setLogStatus(false);
msgSuccess(i18n.global.t('commons.msg.operationSuccess'));
}
}
// For example:
search(): Promise<void> {
const res = await getSettingInfo();
version.value = res.data.systemVersion;
const rstMenuList = getMenuList(res.data);
menuStore.menuList = rstMenuList;
search(); // Remove trailing semicolon
}
openTask(): void {
emit('openTask')
}
Recommendations:
Convert arrow functions to standard function declarations for better readability.
Ensure synchronous functions are properly typed and asynchronous ones handle promises correctly.
Remove unnecessary semicolons at the end of lines where it's not required.
Overall, these changes improve TypeScript type safety and adhere to current best practices. Make sure you adjust the paths according to your local development setup, especially regarding Element Plus components. The rest should be straightforward based on typical refactorings made to maintain clean and well-defined codebases.
@@ -163,6 +185,9 @@ onMounted(() => { | |||
bottom: 0; | |||
left: 0; | |||
overflow: hidden; | |||
.affix { | |||
z-index: 5; | |||
} | |||
} | |||
|
|||
.hideSidebar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provided code is generally clean and well-structured, with some improvements suggested:
Suggestions:
-
Enhanced Sidebar Interactivity:
- Added
@mouseenter
and@mouseleave
events to show/hide a button that can be used to toggle the sidebar's visibility without being directly clicked. This enhances user experience by providing an alternative way to interact.
- Added
-
Optimized CSS Flexbox Usage:
- Updated
.el-button
styles for positioning to ensure it is positioned correctly relative to the collapsed state (var(menu-store.collape)
).
- Updated
-
Added Conditional Styling for Affix Component:
- The
affix
component is now given a higherz-index
, allowing its content to appear above other elements (e.g., main application area) when active within the sidebar.
- The
-
Minor Refactoring:
- Renamed
collapseButtonShow.value = false;
to maintain consistency in naming convention and style clarity.
- Renamed
These enhancements aim to improve both performance and usability of the sidebar while maintaining accessibility and responsiveness.
526ac5f
to
dfa4111
Compare
|
||
.dropdown-item:hover { | ||
background: var(--el-menu-item-bg-color-active); | ||
} | ||
</style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several areas in the provided code that can be improved or optimized:
-
Component Imports: The import statements at the top of the script can be simplified to include only what's necessary.
-
Code DRY (Don't Repeat Yourself): You have repeated logic for loading and checking tasks multiple times. Consider creating a reusable function for this purpose.
-
Dynamic Icon Handling: Instead of using conditional rendering for icons based on
isCollapse
, consider styling them differently when applicable. -
Refactored Event Emitter: Ensure proper binding and lifecycle handling for event emitters like
emit('openTask')
. -
Popper Configuration: Simplify the popover configuration and use default options where possible.
Here’s an updated version with these improvements considered:
<!-- Updated template -->
<template>
<div>
<el-popover
placement="bottom-end"
:show-arrow="false"
:offset="0"
:width="240"
trigger="click"
popper-class="custom-popover-dropdown"
@input="(visible) => updateMenuVisibility(visible)"
>
<template #reference>
<div class="el-dropdown-link" v-if="!menuStore.isCollapse">
<svg-icon class="icon left-icon" iconName="p-gerenzhongxin1"/>
{{ loadCurrentName() }}
<i class="el-icon-caret-bottom el-dropdown__arrow"></i>
</div>
<div v-else class="el-dropdown-link">
<svg-icon class="icon" iconName="p-gerenzhongxin1"/>
</div>
</template>
<!-- Popover content here -->
</el-popover>
</div>
</template>
<script setup lang="ts">
import { computed, ref } from 'vue';
import SvgIcon from '@/components/CommonSvg.svg';
import { MenuStore } from '@/store';
import { DropdownInstance } from 'element-plus';
const menuStore = MenuStore();
// Load nodes and initialize task count
loadNodesAndCheckTasks();
function loadNodesAndCheckTasks() {
Promise.all([
listNodeOptions(),
countExecutingTask(),
]).then(([nodeRes, taskRes]) => {
// Handle node data
if (nodeRes.statusCode === 200 && nodeRes.data) {
nodes.value = [...(nodeRes.data || [])];
setDefaultNode(globalStore.currentNode ?? 'local');
}
// Set initial task count
if (taskRes.statusCode === 200) {
taskCount.value = taskRes.data || 0;
}
}).catch(console.error);
}
function setDefaultNode(targetNode: string): void {
if (targetNode === '') return;
for (let i = 0; i < nodes.value.length; i++) {
if (nodes.value[i].name.startsWith(`node-${targetNode}`)) {
globalStore.setCurrentNode(nodes.value[i].name);
return;
}
}
globalStore.setCurrentNode('local');
}
function handleLocalClick(): void {
globalStore.setCurrentNode('local');
window.location.reload();
}
function handleChangeNode(name: string): void {
const currentNode = getMenuStoreCurrentValue().name;
// Compare versions conditionally
const isCompatibleVersion = compareVersions(currentNode.version, name);
if (!currentNode.status?.includes("Healthy") || isCompatibleVersion !== undefined) {
MsgError(i18n.t(isCompatibleVersion ? "setting.versionHigher": "setting.versionLowerr", [name]));
return;
}
globalStore.setCurrentNode(name as typeof nodes[0]["name"]);
window.location.reload();
}
function loadCurrentName(): string {
const currentValue = getMenuStoreCurrentValue();
return currentValue.name === 'local' ?
i18n.global.t('common.node.master') :
currentValue.displayName
}
function getSelectedNodeFromList(): Record<string, number> | null {
let selectedNodes: Record<string, number> | null = {};
nodes.value.forEach(node => {
Object.entries(selectedNodes).forEach(([oldKey, oldVal], index) => {
// Remove outdated nodes
if (!Object.keys(node.available_features).length ||
!selectedNodes.hasOwnProperty(oldKey)){ // Node key not found
delete selectedNodes[oldKey];
} else { // Key found but values differ -> keep new value
if(Object.values(oldVal)[0] !==
node.available_features.filter(feature => feature.isEnabled)
.map(({displayName})=> displayName)[0]){
console.log(typeof Object.keys(oldVal)[0]);
Object.keys(selectedNodes)[index]
= {...Object.assign(Object.create(null),{newKey:Object.values(selectedNodes)[index]});}
}
}
});
selectedNodes[node.id as keyof Record<string,string>] =
{[`${node.id}`]:{
name:node.name?node.name:'',
status:`${node.health}, available features ${JSON.stringify(
node.available_features.map((feature)=> feature.enabled?"Enabled":"Disabled"))}}`
}};
return selectedNodes
});
return selectedNodes??{};
}
// Vue component setup functions and events remain unchanged
</script>
<style scoped lang="scss">
/* ... existing styles ... */
</style>
Summary Changes:
- Imports: Simplified and corrected the imports.
- Reused Functions: Removed redundant code, especially around fetching and processing tasks.
- Conditional Icons: Refactored icon visibility and positioning.
- Event Emitters: Correctly defined event emitter
updateMenuVisibility
and used it appropriately. - Documentation Fixes: Improved comments and docstrings throughout.
- Logic Optimization: Minimized direct comparisons between arrays by filtering keys directly.
These changes should make the code more maintainable and efficient while ensuring all functionalities continue working correctly.
font-weight: 500; | ||
color: var(--el-color-primary); | ||
height: 38px; | ||
} | ||
.ico { | ||
height: 20px !important; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some areas where improvements could be made:
Code Improvements
-
Code Duplication: The
loadCurrentName
function and thenodes.map(item => item.name)
logic have redundant parts when determining what to display. -
Unnecessary State Updates: The state updates for
taskCount
in each component can be combined into one parent component if needed, usingref
. -
Re-fetching Nodes on Resize: This should not happen every time the window resizes due to performance considerations.
-
Logging Out Logic: Simplify the logout process with fewer dependencies.
-
Optimization of Task Count Update: Ensure that task count is checked less frequently.
-
Event Emission Optimization: Emit events only necessary changes rather than re-rendering unnecessary components often.
-
SVG Icon Usage: Consider replacing SVGs with their
<i>
tags if possible for better compatibility and potential speed optimizations since these will render inline instead of being loaded from an SVG file. -
State Management: Use Vuex store more extensively to centralize data management across the application.
-
Global Methods: Avoid attaching methods directly to HTML elements where they might interfere with other functionalities unless absolutely necessary.
-
Styling Changes: Keep CSS styles minimalistic and avoid overly complex declarations whenever possible to improve rendering speed and SEO benefits.
-
Testing: Implement comprehensive unit tests to ensure all functionality works as expected before deploying any significant changes.
-
Linting and Formatting: Adhere closely to linting rules provided by frameworks like Prettier to maintain coding standards uniformly.
By addressing these points, the application's architecture becomes cleaner, more efficient, and easier to understand and maintain.
@@ -163,6 +185,9 @@ onMounted(() => { | |||
bottom: 0; | |||
left: 0; | |||
overflow: hidden; | |||
.affix { | |||
z-index: 5; | |||
} | |||
} | |||
|
|||
.hideSidebar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code provided contains several improvements and adjustments to enhance functionality and readability:
-
Variable Initialization: Added
const collapseButtonShow = ref();
outside the function scope but within the script tags. -
Dynamic Menu Width: Changed
width: var(--panel-menu-width) !important;
in.app-sidebar
CSS to make it responsive based on screen density using media queries:@media (min-width: 1024px) { .app-sidebar { width: calc(var(--panel-menu-width) * 0.75); } }
-
Improved Button Visibility During Collapsing:
- Used conditional rendering with
v-if="collapseButtonShow"
for displaying the collapse button only when hovered over. - Moved the style into the affix component for better separation of concerns:
<el-affix v-if="collapseButtonShow" :offset="124" class="affix"> <el-button size="small" circle :style="{ 'margin-left': menuStore.isCollapse ? '60px' : '165px', position: 'absolute' }" :icon="menuStore.isCollapse ? 'ArrowRight' : 'ArrowLeft'" plain @click="handleCollapse()" ></el-button> </el-affix>
- Set
z-index: 5;
exclusively for.affix
to ensure it always appears above other content.
- Used conditional rendering with
-
Responsive Design:
- Adjusted the width calculation inside
onMounted()
to maintain responsiveness across different screen sizes.
- Adjusted the width calculation inside
-
Additional Error Handling:
- Added an alert if theme switching fails:
await switchTheme(newColor).catch((error) => { alert('Error switching themes:', error.message); });
- Added an alert if theme switching fails:
By making these changes, the application should be more user-friendly and performant on various devices and windows sizes.
|
No description provided.