Skip to content
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

Component | Graph: Multiple node selection #395

Merged
merged 2 commits into from
Jun 25, 2024

Conversation

reb-dev
Copy link
Collaborator

@reb-dev reb-dev commented May 28, 2024

This PR introduces brushing to the Graph component to allow users to select and drag multiple nodes at once.

How it works:

Brushing mode is active when the shift key is pressed. This allows for panning, zooming and single-node dragging behavior to function independently and without interference from this new functionality. The crosshair cursor on hover indicates that the brush is able to use.

When making a selection, drag the brush over a region to group those nodes. The nodes within the selection range will be highlighted. Upon release of the mouse, the nodes within the selection can be dragged as a group.

If the shift key is released during any point, the selection resets and the brush is inactive until is it is enabled again.

Video example showcasing zooming, panning, single-node dragging and multi-node dragging in various sequences:

Screen.Recording.2024-06-06.at.10.28.23.AM.mov

New Config Properties

  • disableBrush - to disable this behavior
  • onNodeSelectionBrush(nodes: GraphNode<N,L>[], event: D3BrushEvent) - callback when the brush area is being selected
  • onNodeSelectionDrag(nodes: GraphNode<N,L>[], event: D3DragEvent) - callback for the dragging of the selection

CSS Variables

--vis-graph-brushed-node-stroke-color;
--vis-graph-brushed-node-label-text-color;
--vis-graph-brushed-node-icon-fill-color;

Ideally we should be able to add more customization options here, I just wanted to keep it simple while we ensure the functionality is stable.

Other Addition: selectedNodeIds

Although not directly related to this feature, being able to manually provide multiple selectedNodeId values has been requested, so I've added it. Note that selectedNodeId is still a valid config property but in the future we should probably deprecate it in favor of this one.

@reb-dev reb-dev assigned reb-dev and unassigned reb-dev May 28, 2024
@reb-dev reb-dev requested a review from lee00678 May 29, 2024 00:00
Copy link
Contributor

@rokotyan rokotyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reb-dev Awesome feature!

Quick look at the code, looks good. I'm not sure if the nodes selected by the brush need to have their own "selected" state (blue outline), or can they just look like a regular selection? If you think we need to keep the "brushed" state, let's add it to the node _state as well. I'm currently working on a feature that will allow users specify their own node rendering functions and they'll need to implement these states in their code too.

Also I don't think the brush needs to remain visible after selection. To me it looks a bit off, that the node outline is outside of the brush rectangle.

SCR-20240529-mqlm

(Sorry, instead of quote reply I accidentally edited @rokotyan's comment. Reverting it back -Qian)

@@ -116,7 +120,11 @@ export class Graph<
}

public get selectedNode (): GraphNode<N, L> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reb-dev I agree with your comment regarding marking the selectedNode things as deprecated. You can use this jsdoc tag: https://jsdoc.app/tags-deprecated

@rokotyan
Copy link
Contributor

@reb-dev Missed some of your comments

  1. Trigger key. I think it's a good idea to make it configurable
  2. Dragging vs. Moving. I was expecting the brush to disappear after selection leaving the user with a bunch of selected nodes they can drag. Do you have a use case in mind to keep the brush visible?
  3. Brush handles. This depends on 2. Personally, I would just hide the brush after selection. But if we decide to keep it, I think we should show the handles (this will solve the node outline problem)

@lee00678
Copy link
Collaborator

@reb-dev This is great!

In terms of trigger key: Agreed on making it configurable, but I think we could have a separate task for that and keep this PR small?

@lee00678
Copy link
Collaborator

Also I don't think the brush needs to remain visible after selection. To me it looks a bit off, that the node outline is outside of the brush rectangle.

SCR-20240529-mqlm

Agreed. I think just somehow highlight the nodes to indicate they are selected is better than keeping the brush area.

@reb-dev
Copy link
Collaborator Author

reb-dev commented May 30, 2024

@rokotyan @lee00678 thanks for the feedback!

I guess my thought process for keeping the brush was that there should be some indicator of the draggable range besides the highlighted nodes, because in its current state you can select anywhere within the brush area and drag the nodes. But I agree it would look better without it. I'll adjust it so that the only the nodes can drag, not the area itself.

@reb-dev
Copy link
Collaborator Author

reb-dev commented May 30, 2024

@rokotyan

I'm not sure if the nodes selected by the brush need to have their own "selected" state (blue outline), or can they just look like a regular selection?

I thought about this, but what about the case when users provide selectedNodeId or selectedNodeIds to the graph's config? Wouldn't having the brushed nodes look the same as those be confusing? Or are you suggesting we disable brushing but keep multi-dragging for those nodes when selectedNodeIds is provided?

Right now multi-dragging can only occur while the trigger key is pressed, but if we want the above functionality then I'll need to change that so that selected nodes can always be draggable as a unit regardless of how the nodes are selected (through brushing or config property). I'm not opposed to this, but I have some concerns:

  1. Is there ever a case where a user would want to be to single-drag a selected node despite providing multiple values to selectedNodeIds?
  2. Maybe users would want both brushing and manual configuration with selectedNodeIds. For example, let's say you want the brushed selection to update what you pass to that config property with the onNodeSelectionBrush callback.
  3. If we keep brushing and custom selected ids, how do we reconcile the difference between what the user passes and what is actually in a "selected" state?
  4. Providing custom selection ids grays out all the nodes except those and the nodes connected to it. Should those nodes be included in the drag too, or just the active selection? And should we style brushed selections the same way?

@reb-dev
Copy link
Collaborator Author

reb-dev commented May 30, 2024

In terms of trigger key: Agreed on making it configurable, but I think we could have a separate task for that and keep this PR small?

@lee00678 Sounds good to me, I'll hold off on adding that.

@reb-dev reb-dev force-pushed the feat/graph-node-multiselection branch from 5e9cb4a to abdbf0e Compare June 5, 2024 22:43
@reb-dev reb-dev force-pushed the feat/graph-node-multiselection branch from abdbf0e to 72ce873 Compare June 6, 2024 17:20
@reb-dev
Copy link
Collaborator Author

reb-dev commented Jun 6, 2024

Changes since original PR:

  • The brush is no longer visible while dragging
  • Changed the type of event from brush(mode === drag) to a regular node drag so multi-drag can only be triggered by dragging a brushed node (not links, background or anywhere within the brush selection like before)
  • Added a _brushed state to nodes

I've also updated the video in the description above.

@reb-dev reb-dev merged commit 3f03920 into main Jun 25, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants