feat(data-modeling): add add field button to collection node COMPASS-9697 #7221
Conversation
Anemy
added
the
feature flagged
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -145,12 +155,14 @@ export const getFieldsFromSchema = ( | |||
| export function collectionToDiagramNode( | |||
| coll: Pick<DataModelCollection, 'ns' | 'jsonSchema' | 'displayPosition'>, | |||
| options: { | |||
| onClickAddNewFieldToCollection?: () => void; | |||
There was a problem hiding this comment.
This is starting to package way to much very UI specific stuff here, also forcing us to keep properties that are required for the view part optional in the interface because other parts where we call this method just need to get the basic converted method. Can we split this method in two parts and keep anything that is not the base conversion of the node type out of this one?
There was a problem hiding this comment.
I split them up and gave a descriptive name for the part as it's only used for the node layout.
I've been thinking about how we can restructure some of this rendering. Soon we'll be adding more items and configuration around how nodes and fields are rendered, and at the moment it doesn't really lend itself to readability and lacks some structure. We're also creating a lot of objects and react components on the fly. Moving some of the node and edge specific things out of diagram-editor into their own components or hooks may help at least short term.
I don't want to over invest now on making something more performant or refactoring this substantially. I think what we have here is still fairly simple so we can move it around easily if we need.
We could create some providers/context in the diagramming package around the node id, something like what's mentioned in this comment: mongodb-js/diagramming#112 (comment)
Then we could pass a generic NodeAction component here and use the context there which could make things a bit cleaner. I'm thinking that's not for this pr though. I'll give it a think and bring it up in the next sync we do.
There was a problem hiding this comment.
All good points and makes sense to not over commit to something right away! I think this one is just a very clear case already where the cleanup seems to make things a bit more reusable and the types closer to reality and stricter so thanks for taking some time to restructure this!
There was a problem hiding this comment.
I'm doing a bit of refactoring around these parts in #7218, so I appreciate not going full in on the refactor atm 😅
paula-stacho
force-pushed
the
COMPASS-9697-add-field-to-collection-data-modeling
branch
from
862afc1 to
bced8ba
Compare
COMPASS-9697
add.field.mp4
As we don't have the field drawer and field selection yet I didn't add any logic around making the new field selected. We'll want to do that once we have field selection in.