-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Loop body supports zooming in and out #4099
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 |
| props.nodeModel.setHeight(height.value) | ||
| } | ||
| } | ||
| </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 div element added at line 4 should have a closing tag (
</div>). - Remove the commented-out
showiconref definition since it's not used in the code snippet provided. - Ensure that
stepContainerHeight,inputContainerHeight, andoutputContainerHeightproperties ofprops.nodeModel.graphModelare set before using them to changeheightvalue. - Consider replacing hardcoded values with dynamic calculations based on screen dimensions and node properties.
- Use TypeScript interfaces instead of plain objects when defining reactive prop types for better typing support.
- Replace the placeholder comment inside
if (enlarge.value)with actual logic for animating the step container resizing effect.
| } | ||
| } | ||
| export default { | ||
| type: 'loop-body-node', |
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 code looks largely valid and well-organized. It defines a LoopBodyModel class that extends an AppNodeModel. The method for setting height (setHeight) updates the node's properties and propagates the change to the relevant edges. However, there are a few minor suggestions:
-
Type Annotations: Ensure you have appropriate TypeScript type annotations, especially for array variables like
anchors,outgoing.edges, andincoming.edges. -
Variable Naming Consistency: While not strictly necessary, it might be helpful to maintain consistent naming conventions or documentation on what each property represents.
-
Error Handling: Consider adding error handling if the
updatePathByAnchor()functions could throw exceptions, particularly given they modify edges directly. -
Comments: Adding comments explaining complex logic or decisions can help future developers (or perhaps你自己 in the distant future) understand the purpose of certain parts of the codebase more easily.
Here is the updated code with some suggested improvements:
@@ -36,6 +36,18 @@ class LoopBodyModel extends AppNodeModel {
getAnchors(): any[] {
return this.anchors;
}
+ setHeight(height: number): void {
+ if (!Number.isInteger(height)) {
+ throw new Error('Height must be an integer');
+ }
+
+ this.properties['height'] = height;
+
+ this.outgoing.edges.forEach((edge: any) => {
+ edge.updatePathByAnchor();
+ });
+
+ this.incoming.edges.forEach((edge: any) => {
+ edge.updatePathByAnchor();
+ });
+ }
}In summary, the code itself seems functional, but improving readability and robustness would provide better overall quality and maintenance.
| <div ref="containerRef" @wheel.stop style="height: 100%; width: 100%"></div> | ||
| </LoopBodyContainer> | ||
| </template> | ||
| <script setup lang="ts"> |
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 updated code changes the width of the .containerRef to 100%. This is generally appropriate for making the container take up its full available space, especially if it's inside another element with flexible sizing.
Overall, this change seems reasonable within typical use cases, but there aren't any noticeable issues that would prevent the component from functioning correctly as intended. If you anticipate scrolling in different directions or need additional customization based on specific context, further adjustments might be necessary. Otherwise, your changes should work well.
feat: Loop body supports zooming in and out