-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: Intent node height #4113
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: Intent node height #4113
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 |
console.log(y - height / 2 + FORM_ITEMS_HEIGHT + 100/ 2) | ||
} | ||
} | ||
return anchors |
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.
- Line 26: The line
const FORM_ITEMS_HEIGHT = 382
should be updated toconst FORM_ITEMS_HEIGHT = 397
. - Potential Issue with Calculation in Line 44: The calculation
y - height / 2 + FORM_ITEMS_HEIGHT + h + element.height / 2
is slightly off due to incorrect indexing logic (h
). This could lead to misplacement of elements. - Optimization Suggestions:
- Variable Naming: Replace
FORM_ITEMS_HEIGHT
with a more descriptive name likeFORM_ROW_HEIGHT
. Also, consider renamingwidth
,height
,x
, andy
to make code self-explanatory.
- Variable Naming: Replace
Here's the corrected and optimized version:
@@ -48,19 +48,20 @@ class IntentModel extends AppNodeModel {
if (branch_condition_list) {
- const FORM_ROW_HEIGHT = 397 // 上方表单占用高度(新增或更正)
+ const FORM_ROW_HEIGHT = 418; // 使用更高的表单项高度以适应新的需求
for (let index = 0; index < branch_condition_list.length; index++) {
const element = branch_condition_list[index];
- const rowHeightOffset = index === branch_condition_list.length - 1 ? 19 : 0; // 每一行的偏移量,避免最后一行超出边界
+
anchors.push({
x: x + width / 2 - 5,
y: showNode
- ? y - height / 2 + FORM_ROW_HEIGHT + 100/ 2 + rowHeightOffset
+ ? y - height / 2 + FORM_ROW_HEIGHT + Math.min(1, index % 2) * 30
: y + 5,
id: `${id}_${element.id}_right`,
type: 'right'
});
+ // Debugging log can remain here for checking purposes
}
}
return anchors;
Key Changes Made:
- Updated the formula for calculating the position of elements based on the number of rows using
index % 2
instead of hardcoding offsets directly. This ensures that every second row has additional spacing for better layout consistency. - Renamed some variables for clarity, such as
rowHeightOffset
to reflect its purpose better. - Adjusted the default offset for non-last rows and added comments in the debug logging section to keep track of the positions being calculated.
if (form_data.value.branch.length != new Set(form_data.value.branch.map((item: any) => item.content)).size) { | ||
throw t('views.applicationWorkflow.nodes.intentNode.error2') | ||
} | ||
}).catch((err: any) => { |
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.
No apparent errors or issues were found with the provided code. However, there are some optimization and readability improvements that can be made:
- Remove Duplicate Conditional Logic: In
validate()
after the promise chain resolves, the logic that checks for duplicate branch contents is redundant because if there's an error during validation (throw ...
), it won't reach this point.
const validate = () => {
return Promise.all([
nodeCascaderRef.value ? nodeCascaderRef.value.validate() : Promise.resolve(),
IntentClassifyNodeFormRef.value?.validate(),
]).then(() => {
// If form is valid, perform additional checks here
});
};
- Simplify Set Construction: For constructing the Set to check for duplicates, you don't need to use a map first. You can directly use the array filter method.
const validate = () => {
return Promise.all([
nodeCascaderRef.value ? nodeCascaderRef.value.validate() : Promise.resolve(),
IntentClassifyNodeFormRef.value?.validate(),
].reduce((resolve, validator) => validator.then(resolve), Promise.reject()))
.then(() => {
const uniqueContentsSet = new Set(form_data.value.branch.map((item: any) => item.content))
if (uniqueContentsSet.size !== form_data.value.branch.length) throw t('views.applicationWorkflow.nodes.intentNode.error2');
else return null; // Indicating all good
})
};
// Then handle rejection
.catch((err: any) => {});
-
Ensure Function Names Consistency: Naming functions like
addComponentBranch()
might imply side-effects but doesn't match its purpose. Consider renaming it to something more descriptive likeaddClassificationItem
. -
Review Component Hierarchy: Since component sizes increase significantly, ensure that your components maintain optimal performance and adhere to Vue.js best practices such as lifecycle hooks properly implemented in child components.
These changes will help in cleaner code with fewer unnecessary operations.
fix: Intent node height