Skip to content
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

feat: Menu enables vue-router mode #7681

Merged
merged 1 commit into from
Jan 9, 2025
Merged

feat: Menu enables vue-router mode #7681

merged 1 commit into from
Jan 9, 2025

Conversation

lan-yonghui
Copy link
Member

No description provided.

Copy link

f2c-ci-robot bot commented Jan 9, 2025

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.

overflow-wrap: break-word;
line-height: normal;
}

.sidebar-container {
position: relative;
display: flex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No significant issues were found; however, there are some minor improvements that can be made to enhance readability and maintainability:

  1. Simplify Props Definition: The menuRouter prop has been removed as it's not being used inside the component. This line of code should be commented out if you want to remove it permanently.

-defineProps({

  • menuRouter: {
  •    type: Boolean,
    
  •    default: true,
    
  •    required: false,
    
  • },
    -});

2. **Remove Extra White Space in CSS Rule**: There seems to be extra whitespace at the beginning of the `.custom-menu .el-menu-item` rule, which might lead to unintended behavior. It should be cleaned up.

   ```diff
--.custom-menu .el-menu-item {
-    white-space: normal !important;
-    word-break: break-word;
-    overflow-wrap: break-word;
-    line-height: normal;
 }

These changes will make the code cleaner and more readable without introducing any functionality loss.

@@ -36,7 +36,7 @@
</template>
</back-button>

<span v-else class="flex justify-between items-start sm:items-center flex-col sm:flex-row">
<span v-else>
{{ title }}
<span v-if="slots.buttons">
<el-divider direction="vertical" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no visible issue in the code snippet provided between lines 36 and 40 of the difference file. The changes appear to be simply refactoring the v-if statement, which should not affect functionality or layout.

If you want me to perform more thorough checks or provide additional suggestions based on broader code context or specific requirements, please let me know!

@@ -2,7 +2,7 @@
<div :class="classObj" class="app-wrapper" v-loading="loading" :element-loading-text="loadingText" fullscreen>
<div v-if="classObj.mobile && classObj.openSidebar" class="drawer-bg" @click="handleClickOutside" />
<div class="app-sidebar" v-if="!globalStore.isFullScreen">
<Sidebar @menu-click="handleMenuClick" :menu-router="!classObj.openMenuTabs" />
<Sidebar @menu-click="handleMenuClick" />
</div>

<div class="main-container">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an inconsistency between the conditional rendering of both Sidebar components based on the values of classObj.mobile, classObj.openSidebar, and globalStore.isFullScreen. In one instance (v-if="!globalStore.isFullScreen"), you're conditionally showing the Sidebar while also checking if either classObj.mobile or classObj.openSidebar is true. This could result in multiple instances of the Sidebar being rendered in certain scenarios.

Here's a revised version with consistent logic:

@@ -2,7 +2,7 @@
     <div :class="classObj" class="app-wrapper" v-loading="loading" :element-loading-text="loadingText" fullscreen>
         <div v-if="classObj.mobile && classObj.openSidebar" class="drawer-bg" @click="handleClickOutside" />
         <div class="app-sidebar" :key="sidebarKey"
-          v-if=" !classObj.mobile || globalStore.isFullScreen">
+             v-if=" globalStore.isFullScreen || (classObj.mobile && classObj.openSideBar) ">

In this revision, I've consolidated the conditions so that only one instance of Sidebar is displayed under normal usage where neither classObj.mobile nor globalStore.isFullScreen is true. The sidebar remains visible when the screen is not full but user settings permit it to be open using classObj.openMenuTabs.

Additionally, adding a unique key (:key="sidebarKey") helps React manage re-rendering correctly for dynamic content updates. Ensure that there exists at least one initial state value assignable to sidebarKey.

After making these changes, ensure that your Vuex store (globalStore) correctly stores the appropriate states related to mobile view toggles and sidebars.

Copy link

sonarqubecloud bot commented Jan 9, 2025

Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@wanghe-fit2cloud
Copy link
Member

/approve

Copy link

f2c-ci-robot bot commented Jan 9, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wanghe-fit2cloud

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot bot added the approved label Jan 9, 2025
@f2c-ci-robot f2c-ci-robot bot merged commit b9be79b into dev Jan 9, 2025
6 checks passed
@f2c-ci-robot f2c-ci-robot bot deleted the pr@dev@feat_menu branch January 9, 2025 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants