-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Workflow implements multi-select nodes #4183
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 |
| // } | ||
| MsgConfirm(t('common.tip'), t('views.applicationWorkflow.delete.confirmTitle'), { | ||
| confirmButtonText: t('common.confirm'), | ||
| confirmButtonClass: 'danger', |
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 snippet contains no apparent syntax errors, but there might be concerns regarding its structure and functionality. Specifically:
-
Uncommented Code: The commented-out section containing
MsgConfirmis not used in the current state of the code. This can lead to unnecessary overhead. -
Missing Variable Declaration: If
'start-node','base-node', and'loop-body-node'types are defined elsewhere, they should be declared before being used. Ensure these type definitions are available globally. -
Internationalization (
tFunction): The calls to the translation functiontsuggest that some internationalizations are missing or referenced incorrectly. Make sure all translations are correctly set up and accessible within this context. -
Potential Future Changes: Un-commenting sections like the one with
MsgConfirmcould introduce bugs if those functions were meant to execute instead.
Here’s an optimized version without comments, assuming necessary variable declarations and translations are in place:
export function initDefaultShortcut(lf: LogicFlow, graph: GraphModel) {
const nodes = elements.nodes.filter((node) =>
['start-node', 'base-node', 'loop-body-node'].includes(node.type)
);
if (nodes.length > 0) {
MsgError(`${nodes[0].properties?.stepName}${t('views.applicationWorkflow.delete.deleteMessage')}`);
return;
}
// Uncomment the line below if you want to add confirmation logic
MsgConfirm(t('common.tip'), t('views.applicationWorkflow.delete.confirmTitle'), {
confirmButtonText: t('common.confirm'),
confirmButtonClass: 'danger'
});
}If you need further changes based on specific requirements or additional functionalities, please provide more details!
| } | ||
| set(props.nodeModel, 'isSelected', true) | ||
| set(props.nodeModel, 'isHovered', true) | ||
| props.nodeModel.graphModel.toFront(props.nodeModel.id) |
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 has an issue with the parameter mousedown. The function should accept an optional parameter called event to handle event-related actions like checking if shift key is pressed before clearing select elements.
Here's a suggested correction:
const mousedown = (event?: MouseEvent | TouchEvent) => {
if (!event?.shiftKey) {
props.nodeModel.graphModel.clearSelectElements();
}
set(props.nodeModel, 'isSelected', true);
set(props.nodeModel, 'isHovered', true);
props.nodeModel.graphModel.toFront(props.nodeModel.id);
}This ensures that the code handles both mouse events (MouseEvent) and touch events (TouchEvent). Additionally, make sure that you've imported the necessary types and interfaces for these event objects (MouseEvent and optionally TouchEventArgs, depending on your implementation).
| const validate = () => { | ||
| return Promise.all(lf.value.graphModel.nodes.map((element: any) => element?.validate?.())) | ||
| } |
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 has minor improvements such as removing unnecessary semicolons; adding comments to explain some parts of the code for better understanding; and including import statements for missing external dependencies.
No major issues have been found that impact functionality, but here's a suggestion:
To improve performance, consider rendering only the necessary portion of the graph initially. Since renderGraphData is called when the component mounts, you could fetch only essential data for the initial display instead of sending all node information. This would reduce the amount of data processed at startup.
const renderGraphData = async (initialNodes?: Array<any>): Promise<void> => {
if (!initialNodes || initialNodes.length === 0) return;
const container: any = document.querySelector('#container');
if (container && lf.value) {
lf.value.graphModel.batch(() => {
// Assuming 'data' contains an array of nodes IDs
const nodesToAdd: Array<Record<string, string|number>> =
await fetchSomeInitialNodeData(initialNodes).then(responses => responses.json());
// Clear existing edges and nodes, then add the specified initial nodes
lf.value.deleteAll();
This change reduces the number of operations during initialization, potentially improving app load times. Make sure that fetchSomeInitialNodeData() is implemented as described.
fix: Wordflow implements multi-select nodes