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

adds ability to select and remove connections, fixes #107 #116

Merged
merged 4 commits into from
Apr 15, 2024

Conversation

Tokyros
Copy link
Contributor

@Tokyros Tokyros commented Apr 13, 2024

Description

Issue link - #107

This PR implements the ability to select connection via mouse click.
Once selected connections can be delete via backspace key or by clicking the "Delete" button on the Selection Indicator action bar at the bottom.

Users cannot select connections and Node instances at the same time, so selecting either will clear the selection of the other.

Implementation details

  1. To make selection easier I introduces a second invisible <path> element with the same shape and a larger stroke then the connection one, all mouse events are registered to this invisible path.
    Once the mouse is in the range of selection the connection will be highlighted and dashed.
  2. Introduced a new selectedConnections state to differentiate it from the node instances selection.
  3. Removed the pointer-events: none which was preventing any pointer actions on the connections svg, had to also ammend the selection box logic because it broke after removing it.

Copy link

vercel bot commented Apr 13, 2024

@Tokyros is attempting to deploy a commit to the Flyde Team on Vercel.

A member of the Team first needs to authorize it.

Comment on lines 792 to 794
!ids.includes(getConnectionId({from, to})) &&
ids.indexOf(from.insId) === -1 &&
ids.indexOf(to.insId) === -1
Copy link
Contributor

Choose a reason for hiding this comment

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

my bad, but.. boy scout rule

Suggested change
!ids.includes(getConnectionId({from, to})) &&
ids.indexOf(from.insId) === -1 &&
ids.indexOf(to.insId) === -1
!ids.includes(from.insId) &&
!ids.includes(to.insId)

@@ -162,6 +164,7 @@ export type ClipboardData = {
export type GroupEditorBoardData = {
viewPort: ViewPort;
selected: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

now that we have selected connections "selected" becomes (even more) vague

Suggested change
selected: string[];
selectedInstances: string[];

Copy link

vercel bot commented Apr 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
flyde-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2024 6:57pm

@GabiGrin GabiGrin changed the title Issue #107 | cannot remove connections adds ability to select and remove connections, fixes #107 Apr 13, 2024
@GabiGrin
Copy link
Contributor

@Tokyros awesome work!
build isn't passing though - let me know if you need help troubleshooting it

@@ -131,6 +131,14 @@ export function connectionData(fromRaw: any, toRaw: any, delayed?: any): any {
};
}

export const getConnectionId = (connectionData: ConnectionData) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if core is the place for this util - can't see a core usage for it

@GabiGrin
Copy link
Contributor

Screenshot 2024-04-13 at 23 34 04 light mode (can be viewed by setting VSCode to a light theme and reloading) looks bad

@GabiGrin GabiGrin force-pushed the connections-selection branch from 9941b02 to 046c933 Compare April 15, 2024 18:48
@GabiGrin GabiGrin merged commit 2835918 into flydelabs:main Apr 15, 2024
2 of 3 checks passed
@GabiGrin
Copy link
Contributor

Thanks @Tokyros, awesome work 🕺

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.

2 participants