feat/COMPASS-9656 add field selected state and field click handler #112
Conversation
src/types/component-props.ts
Outdated
| id: FieldId; | ||
| nodeId: string; | ||
| }, | ||
| ) => void; |
There was a problem hiding this comment.
I know we said that sending id is fine as we'd provide a default value, but the value might not be unique and I see that the other handlers (Node or Edge) by reactflow actually send the full Node/Edge. Does something speak against doing the same?
There was a problem hiding this comment.
Not that it would help against the non-uniqueness necessarily, unless the field contains something identifying. So I'm more swayed by the consistency now
There was a problem hiding this comment.
I've found keeping interfaces minimal by reducing the passing of unneeded information can be beneficial to reducing coupling and reliance on stale or duplicated data down the line. I think React Flow sends the whole edge or node as a result of them allowing a lot of the diagramming state to be uncontrolled. In our case the consumer of this diagramming package is controlling the data and the state of the fields so they already have all of the information.
|
Looks amazing! 😍 One issue I stumbled upon - when I select a parent, I can't change the selection to the child (also the hover is stuck on the parent) Screen.Recording.2025-08-15.at.15.10.27.mov |
| nodeType={nodeType} | ||
| isHovering={isHovering} | ||
| previewGroupArea={previewGroupArea[getPreviewId(i, name)] || DEFAULT_PREVIEW_GROUP_AREA} | ||
| selectedGroupHeight={selectedGroupHeight?.[getSelectedId(i, name)]} |
There was a problem hiding this comment.
I'm wondering if we even need the name, could this be just index? Or is this better for debugging?
There was a problem hiding this comment.
Good question, I went with name and index to align it with the previewGroupArea implementation. No strong preference there, and I'm not sure on the reason for having name. Maybe @lchans knows? I'm happy to remove it as it would simplify things!
There was a problem hiding this comment.
It handles duplicate names at different depths
|
How difficult would it be to connect this to |
|
@paula-stacho Connecting this to |
…in selection and hover
| let idAccumulator: string[]; | ||
| let lastDepth = 0; | ||
| // Used to build a string array id based on field depth. | ||
| function idFromDepthAccumulator(name: string, depth?: number) { |
There was a problem hiding this comment.
Out of curiosity what's the purpose of this function when we can just define the id ourselves in the story? Do we plan on using this in other places in the future?
There was a problem hiding this comment.
I imagine we'll probably use it when we add stories with field dragging down the line. That being said it was a quick add with the intention of using the already defined fields without changing them. Happy to remove it and define the id ourselves if that would make reading through easier.
External Links
Description
Adds an
onFieldClickprop to the Diagram. This is called when a field is clicked. Also this addsselectedandselectableprops to theNodeField. We have some new styles around the selected state.Notes for Reviewers
Eventually we'll be adding multi-select and multi-field dragging. That'll require some state to track the selected fields. I'm thinking the hook added
useFieldSelectionis going to form a basis for that.A good reference is how react flow does some of their node dragging. It's worth noting though that they have a controlled and uncontrolled approach, and we'll only need to do controlled.
Haven't done much with styled components, so any tips there would be helpful! Was reading https://styled-components.com/docs/basics#passed-props and looking at the existing code, but I may have done a pattern we want to avoid unknowingly.
📸 Screenshots/Screencasts
select.fields.mp4