-
Notifications
You must be signed in to change notification settings - Fork 231
feat(data-modeling): add nested field to object in diagram COMPASS-9742 #7389
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
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.
Pull Request Overview
This PR adds functionality to insert nested fields into object fields within data modeling diagrams, enabling users to add fields directly to object-type fields rather than only at the top level of collections.
- Extends
getNewUnusedFieldName
to support nested field paths for generating unique field names within object fields - Adds
onAddNestedField
action to create nested fields at specified parent paths - Updates diagram component to handle nested field addition through new callback handlers
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/compass-data-modeling/src/utils/schema.ts | Extended getNewUnusedFieldName to traverse nested field paths |
packages/compass-data-modeling/src/utils/nodes-and-edges.ts | Added getBaseFieldsFromSchema function and refactored field creation logic |
packages/compass-data-modeling/src/store/diagram.ts | Added onAddNestedField action to handle nested field creation |
packages/compass-data-modeling/src/components/diagram-editor.tsx | Connected nested field addition functionality to UI callbacks |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
); | ||
|
||
let i = 1; | ||
let fieldName = `field-${i}`; |
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.
A nothing comment but I feel compelled to suggest, why aren't we using "_" instead of "-" since I feel like snake case is the typical format for document keys. But that's just my onion 🧅 (smelly opinion)
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 reason. I think we just did it that way, I'm cool with either, if there's a standard for it let's do it that way! I'll include that change in another pr to avoid another ci run here.
COMPASS-9742
Old pr #7300
I'll do the e2e tests in a separate pr as they are testing more field interactions than just this.