-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Support variable splitting nodes #4181
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
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. DetailsInstructions 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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
||
| defineExpose({ open, close }) | ||
| </script> | ||
| <style lang="scss" scoped></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 provided Vue.js component template and script are syntactically correct and follow Element Plus conventions. However, there are a few areas for improvement and potential optimizations:
-
Template Ref Naming Consistency: Ensure that the input
v-forattribute uses camel case with hyphens for accessibility.<el-form-item v-for="(item, key) in fields" :key="key" :label="t(`fields.${key}.name`)">
2. **Error Handling**: Consider adding error handling to display specific messages for validation errors instead of just "message:".
3. **Language Translation**: The translation function `t()` should be used consistently throughout. For example:
```javascript
message: `${t('validation.required')}: ${t(`fields.${key}.name`)}`,
-
Style Scope Management: Use scoped styles (
<style scoped>) more effectively if necessary. -
Code Comments: Add comments where appropriate to explain complex logic or functionality for better maintainability.
Overall, the structure and functionality meet the requirements specified, but these suggestions can help make the component more robust and user-friendly.
| }) | ||
| </script> | ||
|
|
||
| <style scoped lang="scss"></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 appear to be several issues and areas for improvement in the provided Vue.js component:
Issues:
-
Potential Security Vulnerability:
import { set, cloneDeep } from 'lodash';
Ensure that you trust
set()andcloneDeep(), especially when handling user inputs or sensitive data. -
Duplicated Code:
- The code snippet has similar logic repeated twice in different functions (
refreshFieldListand within the template). - Consider extracting this common functionality into a separate method or function.
- The code snippet has similar logic repeated twice in different functions (
-
Data Integrity Issue:
- The lines like
if (props.nodeModel.properties.node_data.variable_list)can lead to errors ifnode_model.properties.node_datais not defined or does not contain the expected structure. - It's better to safely access properties using optional chaining or default values.
- The lines like
-
Missing Imports:
- Ensure that all components used (
AppIcon,VariableFieldDialog) are properly imported at the top of the file.
- Ensure that all components used (
-
Component Naming Conventions:
- Component names should typically start with a capital letter and use kebab-case unless you have specific naming conventions, which isn't followed here consistently.
-
Scoping:
- Using
ref()incorrectly (e.g.,ref="tableRef"where there’s no actual element bound).
- Using
-
Translation Functions:
- Ensure that
$tcalls resolve without errors, as they might throw an error if translations aren’t loaded correctly.
- Ensure that
-
TypeScript Types:
- Define types for props and other variables to improve maintainability and catch errors during development.
-
Comments and Documentation:
- Add comments to explain complex logic or steps taken in certain methods to make it easier to understand.
Here is an improved version with some of these suggestions:
<template>
<div class="flex-between mb-16">
<h5 class="break-all ellipsis lighter" style="max-width: 80%">
{{ $t('views.applicationWorkflow.nodes.variableSplittingNode.splitVariables', '拆分变量') }}
</h5>
<div>
<el-button link type="primary" @click="openAddDialog">
<AppIcon iconName="app-add-outlined" class="mr-4"></AppIcon>
{{ $t('common.add') }}
</el-button>
</div>
</div>
<el-table
v-if="(props.nodeModel?.properties?.node_data || {}).variable_list?.length"
:data="(props.nodeModel?.properties?.node_data || {})?.variable_list"
class="mb-16"
ref="tableRef"
row-key="field"
>
<el-table-column
prop="field"
:label="$t('dynamicsForm.paramForm.variable.label', '变量')"
width="95"
/>
<el-table-column prop="label" :label="$t('dynamicsForm.paramForm.name.label')" />
<el-table-column :label="$t('common.operation')" align="left" width="90">
<template #default="{ row, $index }">
<!-- ... -->
</template>
</el-table-column>
</el-table>
</template>
<script setup lang="ts">
import AppIcon from '@/components/AppIcon.vue'; // Ensure correct import path exists
import VariableFieldDialog from './VariableFieldDialog.vue';
import { t } from '@/locales';
import { onMounted, ref } from 'vue';
const props = defineProps<{ nodeModel?: any }>();
const tableRef = ref();
const VariableFieldDialogRef = ref();
// Extract duplicated logic into helper function
async function saveFieldsToList(fields: { field: string; label: string }[]) {
try {
fields.forEach(({ field, label }) => {
let existingIndex = inputFieldList.value.findIndex(item => item.field === field);
if (existingIndex >= 0 && fields.length > 1) { // Allow only one result variable
MsgError(`参数重复,${field}`);
}
if (!inputFieldList.value.some(item => item.field === field)) {
inputFieldList.value.push({ field, label });
} else {
inputFieldList.value[existingIndex] = { field, label };
}
});
refreshFieldDisplay(list);
if (fields.includes(resultOption)) {
resetResultField();
}
console.log(inputFieldList);
} catch (error) {
console.error(error)
}
}
function handleSaveChanges() {
const fieldsToSave = [{
label: t("views.businessprocess.designer.variables.result", "结果"),
value: "result",
},
...inputFieldList.value,
];
saveFieldsToList(fieldsToSave);
}
await handleSaveChanges();Key Changes:
- Removed duplicate
refreshFieldListlogic. - Extracted common logic into a helper function
saveFieldsToList. - Used TypeScript types for clearer definition.
- Added initial call to
handleSaveChangesupon component initialization for demonstration purposes (should replace placeholder comment).
Adjustments made according specific requirements and assumptions about your application context. Adjust paths and content based on the real implementation details.
| set(props.nodeModel, 'validate', validate) | ||
| }) | ||
| </script> | ||
| <style lang="scss" scoped></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.
Code Review and Suggestions
-
Variable Naming:
- The names of variables (
form,input_variable, etc.) could be more descriptive to improve readability.
- The names of variables (
-
Template Element:
- The
<template>element is missing an end tag (</template>) which should ideally go before the opening<script setup>block.
- The
-
Form Validation Logic:
- It's good to have a separate method for validation, but ensure it handles edge cases properly. Consider adding some error handling if data fetching fails.
-
Computed Property Efficiency:
- The computed property
formcan be made simpler by directly assigning the default values tonode_model.properties.
- The computed property
-
Code Order:
- The order of code blocks and elements in the template and script sections may not align logically with best practices. Keep related snippets together where possible.
-
Comments:
- Adding comments might help clarify complex parts like the computed property behavior.
-
Avoid Redundancy:
- Ensure that no properties are being assigned unnecessarily or redundantly.
Here is the revised version incorporating some of these suggestions:
<template>
<NodeContainer :nodeModel="nodeModel">
<el-form
@submit.prevent
:model="formData"
label-position="top"
require-asterisk-position="right"
label-width="auto"
ref="variableSplittingRef"
hide-required-asterisk
>
<!-- Form Input -->
<el-form-item
:label="$t('views.applicationWorkflow.nodes.variableSplittingNode.label', '输入变量')"
>
<template #label>
<div class="flex-between">
<div>
{{ $t('views.applicationWorkflow.nodes.variableSplittingNode.label', '输入变量') }}
<span class="color-danger">*</span>
</div>
</div>
</template>
<NodeCascader
ref="nodeCascaderRef"
:nodeModel="nodeModel"
class="w-full"
:placeholder="$t('views.applicationWorkflow.variable.placeholder')"
v-model="formData.inputVariable"
/>
</el-form-item>
<!-- Variable Field Table -->
<VariableFieldTable ref="variableFieldTableRef" :node-model="nodeModel"></VariableFieldTable>
</el-form>
</NodeContainer>
</template>
<script setup lang="ts">
import { computed, ref, onMounted } from 'vue';
import NodeContainer from '@/workflow/common/NodeContainer.vue';
import NodeCascader from '@/workflow/common/NodeCascader.vue';
import VariableFieldTable from '@/workflow/nodes/variable-splitting/component/VariableFieldTable.vue';
// Define Props
const props = defineProps<{ nodeModel: any }>();
// Initial State
const formData = computed(() => ({
inputVariable: [],
variableList: [],
}));
// Validate Function Ref
const VariableSplittingRef = ref();
// Handle Form Submission
async function validate() {
try {
await VariableSplittingRef.value.validate();
// Add additional submission logic here
console.log("Validation successful");
} catch (error) {
console.error("Validation failed:", error);
// Optionally show user feedback about errors
}
}
// Mount Hooks
onMounted(() => {
set(props.nodeModel.properties, 'node_validate', validate); // Use camelCase instead of snake_case
});
</script>
<style lang="scss" scoped></style>';Additional Tips:
- Error Handling: Implement proper error handling when dealing with asynchronous operations such as form submissions or network calls.
- Styling Optimization: Minimize CSS file size and use modern SCSS features to make styles cleaner and more maintainable.
- Accessibility: Ensure that components meet accessibility standards by providing appropriate labels and descriptions for users who interact with assistive technologies.
These changes aim to enhance the readability, functionality, and performance of your Vue.js component.
feat: Support variable splitting nodes