-
Notifications
You must be signed in to change notification settings - Fork 229
feat(data-modeling): node selection and relationship editing store and actions COMPASS-9476 #7118
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
base: main
Are you sure you want to change the base?
Conversation
@@ -24,6 +27,7 @@ export type DiagramState = | |||
next: Edit[][]; | |||
}; | |||
editErrors?: string[]; | |||
selectedItems: { type: 'collection' | 'relationship'; id: string } | null; |
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 root state here is what is actually being selected by the user, sidebar is just driven by this value
if ( | ||
sidePanel.viewType === 'relationship-editing' && | ||
sidePanel.relationshipFormState.modified | ||
) { | ||
const allowSelect = await showConfirmation({ | ||
title: 'You have unsaved chages', | ||
description: | ||
'You are currently editing a relationship. Are you sure you want to select a different collection?', | ||
}); | ||
if (!allowSelect) { | ||
return; | ||
} | ||
} |
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.
I'm not sure this behavior is needed, but when playing around with the feature it seemed like a good idea to add it, otherwise it's easy to lose your form state by clicking around (not sure many people will do this though)
const { result: isValid, errors } = validateEdit(edit); | ||
if (!isValid) { | ||
dispatch({ | ||
type: DiagramActionTypes.APPLY_EDIT_FAILED, | ||
errors, | ||
}); | ||
return; | ||
return isValid; |
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.
Returning the value from action so that other actions using it can adjust their behavior if needed, otherwise there's no indication that the action failed. There is currently no validation in the form and strictly speaking we are fully in control there so we can't actually fail this validation, but it might be useful in the future
// With threshold too low clicking sometimes gets confused with | ||
// dragging | ||
// @ts-expect-error expose this prop from the component | ||
nodeDragThreshold={3} | ||
// @ts-expect-error expose this prop from the component | ||
onNodeClick={(_evt, node) => { | ||
if (node.type !== 'collection') { | ||
return; | ||
} | ||
onCollectionSelect(node.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.
Note to self: don't forget to open tickets for those changes in diagramming package
return r.id === rId; | ||
}); | ||
|
||
if (relationship) { |
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.
Can we add something for the other case (throw or log)?
This patch adds all required item selection state and actions, including relationship editing, UI is just a rough functional version to validate that it works, this is not in scope for this this ticket and will be addressed in the followups.
Kapture.2025-07-17.at.12.09.05.mp4
I'm adding some tests, but this is ready foe review more or less, store changes are as always the most interesting part here, UI will be changed later
COMPASS-9476