-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: role #3229
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: role #3229
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-sigs/prow 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 |
flex-direction: column; | ||
} | ||
} | ||
</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.
The code appears to be generally well-written with proper structure and TypeScript setup, but here's a brief analysis of possible improvements:
Improvements:
-
Component Communication: Ensure that
Member
component receives the correct props (currentRole
) and handles them appropriately. -
State Management:
- Consider using Vuex for state management if you're building a larger application to avoid passing too many complex objects around components.
- Use pinia instead of vuex if your project supports it due to better performance and simplicity.
-
Code Duplication:
- The dropdown menu logic is duplicated in both lists (internal and custom roles). It can be extracted into a separate method for consistency.
-
Loading State Handling:
- Consider implementing a more robust approach to manage loading states when multiple APIs are involved (e.g., use an external library like vue-spinners).
-
Accessibility:
- Ensure that keyboard navigation is accessible by adding appropriate aria attributes to focusable elements.
-
Error Handling:
- Add detailed error handling to provide clear feedback to users about what went wrong during requests, such as network errors or invalid responses.
-
Performance Optimization:
- Consider optimizing the filtering logic to handle large datasets, especially if there's a need to update these filters frequently.
-
Comments and Descriptive Variable Names:
- Enhance comments explaining complex sections of code for clarity.
-
Style Consistency:
- Ensure consistent spacing, font sizes, and other UI elements within the component to maintain uniformity.
-
Internationalization Errors:
- Verify that all translated strings are correctly defined and available in the localizer function
t
.
- Verify that all translated strings are correctly defined and available in the localizer function
These points provide guidance on how to enhance and refine the provided Vue.js template and script section.
padding: 16px 24px; | ||
box-sizing: border-box; | ||
} | ||
</style> No newline at end of file |
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 appear to be some minor issues and optimizations that can be made:
-
Missing Semicolons: Add semicolons after each statement or block of code for better readability.
-
Type Annotations: Consider adding more detailed type annotations where necessary to improve code clarity and maintainability.
-
Arrow Function Syntax: Use arrow functions consistently throughout the script setup for cleaner syntax.
-
Code Structure: Ensure consistent indentation and spacing within the script setup.
Here's the revised code with these suggestions applied:
<template>
<el-scrollbar v-loading="loading">
<div class="p-24 pt-0">
<el-table :data="tableData" border :span-method="objectSpanMethod">
<el-table-column prop="module" :width="130" :label="$t('views.role.permission.moduleName')" />
<el-table-column prop="name" :width="150" :label="$t('views.role.permission.operationTarget')" />
<el-table-column prop="permission" :label="$t('views.model.modelForm.permissionType.label')">
<template #default="{ row }">
<el-checkbox-group v-model="row.perChecked" @change="handleCellChange($event, row)">
<el-checkbox v-for="item in row.permission" :key="item.id" :value="item.id" :disabled="disabled">
<div class="ellipsis" style="width: 96px">{{ item.name }}</div>
</el-checkbox>
</el-checkbox-group>
</template>
</el-table-column>
<el-table-column :width="40">
<template #header>
<el-checkbox :model-value="allChecked" :indeterminate="allIndeterminate" :disabled="disabled"
@change="handleCheckAll" />
</template>
<template #default="{ row }">
<el-checkbox v-model="row.enable" :indeterminate="row.indeterminate" :disabled="disabled"
@change="(value: boolean) => handleRowChange(value, row)" />
</template>
</el-table-column>
</el-table>
</div>
</el-scrollbar>
<div v-if="!disabled" class="footer border-t">
<el-button type="primary" style="width: 80px;" :loading="loading" @click="handleSave()">
{{ $t('common.save') }}
</el-button>
</div>
</template>
<script setup lang="ts">
import { ref, watch, computed } from 'vue';
import type {
RoleItem,
RolePermissionItem,
RoleTableDataItem,
ChildrenPermissionItem,
} from '@/api/type/role';
import RoleApi from '@/api/user/role';
import { MsgSuccess } from '@/utils/message';
import { t } from '@/locales';
const props = defineProps<{
currentRole?: RoleItem;
}>();
const loading = ref(false);
const tableData = ref<RoleTableDataItem[]>([]);
const disabled = computed(() => props.currentRole?.internal); // TODO 权限
function transformData(data: RolePermissionItem[]): RoleTableDataItem[] {
const transformedData: RoleTableDataItem[] = [];
data.forEach(module => {
module.children.forEach(feature => {
const perChecked = feature.permission
.filter(p => p.enable)
.map(p => p.id);
transformedData.push({
module: module.name,
name: feature.name,
permission: feature.permission,
enable: feature.enable,
perChecked,
indeterminate: perChecked.length > 0 && perChecked.length < feature.permission.length,
});
});
});
return transformedData;
};
async function getRolePermission() {
if (!props.currentRole?.id) return;
try {
tableData.value = [];
const res = await RoleApi.getRolePermissionList(props.currentRole.id, loading);
tableData.value = transformData(res.data);
} catch (error) {
console.error(error);
}
}
function handleCellChange(checkedValues: string[], row: RoleTableDataItem): void {
row.enable = checkedValues.length === row.permission.length;
row.indeterminate = checkedValues.length > 0 && checkedValues.length < row.permission.length;
row.permission.forEach(p => {
p.enable = checkedValues.includes(p.id);
});
};
function handleRowChange(checked: boolean, row: RoleTableDataItem): void {
if (checked) {
row.perChecked = row.permission.map(p => p.id);
row.permission.forEach(p => p.enable = true);
} else {
row.perChecked = [];
row.permission.forEach(p => p.enable = false);
}
row.indeterminate = false;
};
const allChecked = computed(() => {
return tableData.value.length > 0 && tableData.value.every(item => item.enable);
});
const allIndeterminate = computed(() => {
return !allChecked.value && tableData.value.some(item => item.enable);
});
function handleCheckAll(checked: boolean): void {
tableData.value.forEach(item => {
item.enable = checked;
item.perChecked = checked ? item.permission.map(p => p.id) : [];
item.indeterminate = false;
item.permission.forEach(p => p.enable = checked);
});
};
const objectSpanMethod = ({ row, column, rowIndex, columnIndex }: any): { rowspan: number; colspan: number } => {
if (columnIndex === 0) {
const sameModuleRows = tableData.value.filter(item => item.module === row.module);
const firstRowIndex = tableData.value.findIndex(item => item.module === row.module);
if (rowIndex === firstRowIndex) {
return { rowspan: sameModuleRows.length, colspan: 1 };
} else {
return { rowspan: 0, colspan: 0 };
}
}
};
watch(
() => props.currentRole?.id,
async (_newVal, _oldVal) => {
await getRolePermission();
},
{ immediate: true },
);
async function handleSave(): Promise<void> {
try {
const permissions: { id: string, enable: boolean }[] = [];
tableData.value.forEach(e => {
e.permission?.forEach(ele => {
permissions.push({ id: ele.id, enable: ele.enable });
});
});
await RoleApi.saveRolePermission(props.currentRole?.id as string, permissions, loading);
MsgSuccess(t('common.saveSuccess'));
} catch (error) {
console.log(error);
}
};
</script>
<style lang="scss" scoped>
.deep(el-checkbox-group).d-flex.flex-wrap.gap-4 {
}
.footer {
width: 100%;
display: flex;
justify-content: flex-end;
padding: 16px 24px;
box-sizing: border-box;
}
</style>
These changes ensure proper formatting, consistency, and potentially improved performance. Make sure to test thoroughly to verify that any new issues are resolved.
} | ||
|
||
defineExpose({ open }) | ||
</script> |
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 you've provided seems to be a Vue.js component using Element Plus. Here are a few points of concern:
-
Duplicate Import Statements: You have two imports for
MsgSuccess
which is unnecessary and could potentially lead to conflicts. -
Variable Naming Consistency: It's good practice to use consistent naming, such as PascalCase for variable names unless they align with existing conventions or specific libraries.
-
Conditional Check for Role ID: The logic for opening the dialog is inconsistent. When passing an item from parent components, it should handle both creating new roles (no role_id) and renaming them (has role_id).
-
Loading State Management: Ensure that the loading state (
loading
) is properly reset after successful operations, especially if multiple submissions occur. -
Error Handling: Implement proper error handling for API calls to
RoleApi.CreateOrUpdateRole
. -
Accessibility Considerations: Ensure that all elements have appropriate accessibility attributes like
aria-labels
andalt
text, depending on their usage. -
Type Definitions: Verify that the types defined in
interface
match those used throughout the code to prevent runtime errors.
Here’s an updated version of the script part based on these considerations:
<script setup lang="ts">
import { ref, reactive } from 'vue';
import type { FormInstance } from 'element-plus';
import { MsgSuccess } from '@/utils/message';
import { t } from '@/locales';
import type { RoleItem, CreateOrUpdateParams } from '@/api/type/role';
import RoleApi from '@/api/user/role';
import { roleTypeMap } from '../index';
const emit = defineEmits<{
(e: 'refresh', currentRole: RoleItem): void;
}>();
const dialogVisible = ref<boolean>(false);
const defaultForm = {
role_name: ""
};
const form = ref<CreateOrUpdateParams>({
...defaultForm,
});
function open(data?: RoleItem) {
if (data) {
const { id, role_name, type } = data || {};
// Use defaults instead of empty strings to maintain consistency
form.value = {
role_name,
role_type: '' + (type !== undefined && typeof type === 'number' ? type : ''),
role_id: id !== undefined && typeof id === 'number' ? id : '',
};
} else {
form.value = { ...defaultForm };
}
dialogVisible.value = true;
}
const formRef = ref<FormInstance>();
// Adjust the validation rules to only trigger blur when necessary
let fieldBlurred = '';
const rules = reactive({
role_name: [
{ required: true, message: `${t('common.inputPlaceholder')}${t('views.role.roleName')}`, trigger: ['blur'] },
],
role_type: [
{ required: true, message: `${t('common.selectPlaceholder')}${t('views.role.inheritingRole')}`, trigger: ['change'] }, // Change to change event since this might trigger dynamically
]
});
const loading = ref(false);
function submit(formEl: FormInstance | undefined) {
if (!formEl) return;
// Resetting the blurred flag here ensures correct feedback behavior
fieldBlurred = '';
formEl.validate(async (valid) => {
if (valid) {
try {
loading.value = true;
const response = await RoleApi.CreateOrUpdateRole(form.value, loading);
MsgSuccess(!form.value.role_id ? t('common.createSuccess') : t('common.renameSuccess'));
emit('refresh', response?.data);
dialogVisible.value = false;
} catch (error) {
console.error("An error occurred", error);
// Optionally display a human-readable/error-capturing message
MsgSuccess(t('views.common.genericOperationFailure'), "danger");
} finally {
loading.value = false; // Reset loading state regardless of success
}
} else {
// Trigger validation error messages manually
document.activeElement.focus();
setTimeout(() => {
// Wait for focus transition before showing error message
document.getElementById(fieldBlurred)?.focus(); // Using elementId for simplicity assuming unique ids per input
});
}
});
}
defineExpose({ open });
</script>
This revision addresses some of the mentioned concerns while maintaining readability and adhering to best practices. Make sure to update other parts of your application accordingly, such as updating any usages or additional styling changes where needed.
feat: role