-
Notifications
You must be signed in to change notification settings - Fork 1
refactor/COMPASS-9877 add editable node handlers and interactions #135
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?
refactor/COMPASS-9877 add editable node handlers and interactions #135
Conversation
line-height: 20px; | ||
`; | ||
|
||
const AddNestedFieldIconButton = styled.button` |
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.
Why do we need to style a button rather than using the IconButton
that LG provides?
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 LeafyGreen one was too large in terms of both size and focus state. I was thinking passing a lot of custom overriding styles would end up in a more broken state down the line if internals in LeafyGreen change and we upgrade.
https://www.mongodb.design/component/icon-button/live-example
); | ||
|
||
return ( | ||
<ObjectTypeContainer> |
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.
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.
Wondering if you could add the Figma link as well in this PR? Thanks!
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.
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.
Added a 🎨 Figma link. I went with a custom button on the object field type add button to make that focus active state look better. We could share the same button here so they both have the same focus styles. How does that sound?
Answering your other question here as well, I went with a custom button and not icon button for the object field type as the leafygreen one was too large and I was thinking passing a lot of custom overriding styles would end up in a more broken state down the line if internals in LeafyGreen change and we upgrade.
External Links
Description
In the tech design for editing data models in Compass we originally opted to move a number of the customizable parts of rendering fields and field types to the consumer of the diagramming package. However we have found that doing this introduces more complexity when react flow needs to render the field or node, passing a react function can be non-serializable state and can cause unintended re-renders. Those unintended re-renders hinder performance and can cause flashes.
This pr adds new callback functions to the diagramming library, which, when supplied, render related actions for fields and nodes. We're removing the custom node actions render function prop and field name render we've added as they're now to be covered internally. Down the line if we need to add custom rendering passed by a consumer we should instead have it be something that's supplied as a generic function which we can then use with react context in the location that react flow renders as to avoid passing around components in field or node data. We should not add react functions to the node or field types that are used by React Flow.
By adding this editing capabilities to this package, we're thinking it may be of more use down the line if other folks want to have similar functionality.
Notes for Reviewers
There is still one passing of a react function in the code base, that's in the
type
field. That will be addressed in a later pr. We will also be adding field name renaming.Field selection (in the story and video below) will causes flashes on slower machines. This is a result of us re-creating all of the nodes, and fields in them, on selection and reflecting their selected state there. To fix this we'll want to have the selected fields supplied with a context that the field components and listen to and update accordingly. I'll create a separate ticket for that. Branch with that work: https://github.com/mongodb-js/diagramming/compare/refactor/COMPASS-9879-update-how-field-selection-is-passed?expand=1 Tested with the stress test and it was working smoooth.
If folks from the migrator side would prefer that these editing components like the plus button to add a field don't make it into this package, we could add generic react props onto the
Diagram
interface, likerenderFieldType
. We would then render them from a react context in theField
. That would have the same intended outcome as this implementation as we would be getting rid of having those functions on the items sent to react flow.📸 Screenshots/Screencasts
node.and.field.interactions.mp4