fix: Resource authorization selection directory checkbox Cancel optimization

[shaohuzhang1]

fix: Resource authorization selection directory checkbox Cancel optimization

[f2c-ci-robot]

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.

Details

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-sigs/prow repository.

[f2c-ci-robot]

[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.

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[shaohuzhang1]

The code appears to be mostly well-written, but there are a few areas that could potentially benefit from improvements:

1. Select Function Handling: The select function is handling folder permissions differently than non-folder items. It's using TreeToFlatten to flatten nested data structures unnecessarily for folders. This can be optimized by removing this step when it doesn't add value.

2. Conditional Checks: The conditional checks and logic around the selection of rows feel somewhat repetitive at line 388. Consider simplifying these conditions if necessary.

3. State Management: Ensure that the state management (multipleSelection, selectObj) is handled correctly within reactive contexts. Using refs might be redundant in some cases where computed properties or store states would suffice.

4. Error Prone Code: There isn't any obvious error-prone code in this snippet, but ensuring that all variables have been properly initialized before being used can prevent runtime errors.

Here's an updated version with some of these suggestions applied:

@@ -55,7 +55,7 @@
         ref="multipleTableRef"
         class="mt-16"
         :data="filteredData"
-        @selection-change="handleSelectionChange"
+        @select="handleSelect"
         :max-table-height="260"
         :row-key="(row: any) => row.id"
         :expand-row-keys="defaultExpandKeys"
@@ -310,21 +311,35 @@ const filteredData = computed(() => {
 })
 
 const multipleSelection = ref([])
+const selectedFoldersWithAllChildrenSelected = new WeakMap(); // Use WeakMap instead of plain object
+function handleSelect(val: any[], active: any) {
   if (active.resource_type === 'folder' && active.child_count > 0) {
     if (!val.some(item => item.id === active.id)) {
       let childIds = getAllChildIds(active.children);
+      if (!selectedFoldersWithAllChildrenSelected.has(active.id)) {
+        selectedFoldersWithAllChildrenSelected.set(active.id, [...childIds]);
+      }
       console.log(childIds);
-        childIds.forEach(item => {
-          multipleTableRef.value.toggleRowSelection(this.$refs.multipleTableRef.rows.find(row => row.id === item), true);
-        });
+        val.push(...childIds.map(id => multipleTableRef.value.getRowById(id)));
       } else if (selectedFoldersWithAllChildrenSelected.get(active.id).length !== childIds.length) {
         selectedFoldersWithAllChildrenSelected.delete(active.id);
         val.forEach(item => { multipleTableRef.value.toggleRowSelection(item, false); });
+        val.push(...childIds.map(id => multipleTableRef.value.getRowById(id)));
       }
       multipleSelection.value = val;
       return;
     }
   }

+  const expandedFolderIds: string[] = [];
+  Object.entries(selectedFoldersWithAllChildrenSelected).forEach(([key, children]) => {
+    const expandedIndex = multipleTableRef.value.expanded.indexOf(Number(key));
+    if (expandedIndex === -1) {
+      multipleTableRef.value.expandRowByKey(Number(key));
+      expandedFolderIds.push(Number(key));
+    }
+  });

   multipleSelection.value = val.filter((item) => !children.includes(String(item.id)))
}

Explanation Changes:

• WeakMap Usage: Replaced the simple object with WeakMap to avoid storing references to objects unnecessarily.
• Expanded Rows Logic: Added logic to expand parent nodes whenever all their child nodes are selected.
• Simplified Conditional Checks: Eliminated repeated code blocks for checking and toggling selections based on resource type.

[shaohuzhang1]

Your code is generally clean and well-structured. Here are some minor improvements and clarifications:

1. Remove Unnecessary Comma: There's an unnecessary comma after pageSizes in the first line of the <el-pagination> tag. Remove it to ensure proper syntax.

Optimizations and Suggestions

2. Avoid Redefinitions: Although not incorrect, there's no need to define a function called handleSizeChange just to call another function (handleCurrentChange). You can directly pass the event handler without defining an additional function.

Here’s the revised version with these adjustments:

<template>
  <!-- Template content here -->
</template>

<script lang="ts">
import { ref, onMounted } from 'vue';
import {
  ElPagination,
} from "element-plus";

export default defineComponent({
  components: {
    ElPagination,
  },
  setup() {
    const paginationConfig = reactive({
      current_page: 1,
      pagesize: 0,
    });

    const appTableRef = ref<Element | undefined>();

    function handleCurrentChange(pageNo) {
      paginationConfig.current_page = pageNo;
      // Add logic to fetch data based on new page number
    }

    function clearSelection() {
      appTableRef.value?.clearSelection();
    }
    
    function toggleRowSelection(row: any, selected?: boolean, ignoreSelectable = true) {
      if (appTableRef.value) {
        appTableRef.value.toggleRowSelection(row, selected, ignoreSelectable);
      }
    }

    function getSelectionRows() {
      return appTableRef.value ? appTableRef.value.getSelectionRows() : [];
    }

    onMounted(() => {
      // Initialization code if needed
    });

    defineExpose({
      clearSelection,
      toggleRowSelection,
      getSelectionRows,
    });
  }
});
</script>

<style scoped>
/* Styles go here */
</style>

These changes make your code slightly more concise and align better with Vue.js conventions.