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.

Sign up for GitHub

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

Jump to bottom

feat(editor): Add remove node and connections functionality to canvas v2 #9602

Merged
merged 10 commits into from
Jun 4, 2024
Merged

feat(editor): Add remove node and connections functionality to canvas v2 #9602

merged 10 commits into from
Jun 4, 2024

Conversation

alexgrozav
Copy link
Member

@alexgrozav alexgrozav commented Jun 3, 2024 •
edited
Loading

Summary

Adds remove node and connections functionality, toolbar and edge actions. Introduces useCanvasOperations for node and connection specific actions, to avoid code pollution in node view v2 component.

Related to #9135

image

Related tickets and issues

https://linear.app/n8n/issue/N8N-7403/add-ability-to-delete-edge-via-delete-key
https://linear.app/n8n/issue/N8N-7402/add-ability-to-delete-node-via-delete-key
https://linear.app/n8n/issue/N8N-7387/add-edge-actions
https://linear.app/n8n/issue/N8N-7401/add-ability-to-delete-node-via-node-actions

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.

    A bug is not considered fixed, unless a test is added to prevent it from happening again.
    A feature is not complete without tests.

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Jun 3, 2024
MiloradFilipovic
MiloradFilipovic reviewed Jun 4, 2024
View reviewed changes
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

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

Nice progress 🙌
Also left a few comments

packages/editor-ui/src/components/canvas/elements/edges/CanvasEdge.vue
/>
<EdgeLabelRenderer>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to have multiple template roots here? If yes, can we use fragments for this? Otherwise, we can just wrap them in a container

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what we're doing here. There's no explicit syntax for fragments in Vue(like <>..</> in React). It's just ESlint not "realizing" this is Vue 3 project. So, I wouldn't add another container as those would add up quickly for each edge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I thought we are not handling the props correctly or something and that's why eslint is complaining. Does it make sense to turn off this rule for editor-ui?

packages/editor-ui/src/components/canvas/elements/edges/CanvasEdge.vue Outdated Show resolved Hide resolved
@MiloradFilipovic
Copy link
Contributor

For me, new nodes are added outside of the viewport

Screen.Recording.2024-06-04.at.11.36.05.mov

@alexgrozav
Copy link
Member Author

For me, new nodes are added outside of the viewport

Screen.Recording.2024-06-04.at.11.36.05.mov

It's a known issue: https://linear.app/n8n/issue/N8N-7400/compute-node-position-when-creating-new-nodes

@MiloradFilipovic MiloradFilipovic self-requested a review June 4, 2024 11:27
MiloradFilipovic
MiloradFilipovic approved these changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback!

@cypress Cypress
Copy link

cypress bot commented Jun 4, 2024

1 flaky test on run #5299 ↗︎

0 354 0 0 Flakiness 1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 alexgrozav 🗃️ e2e/*
Project: n8n Commit: 5f66b65c5e
Status: Passed Duration: 04:46 💡
Started: Jun 4, 2024 11:33 AM Ended: Jun 4, 2024 11:38 AM
Flakiness  cypress/e2e/5-ndv.cy.ts • 1 flaky test

View Output Video

Test Artifacts
NDV > should not retrieve remote options when required params throw errors Screenshots Video

Review all test suite changes for PR #9602 ↗︎

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 4, 2024

✅ All Cypress E2E specs passed

@alexgrozav alexgrozav merged commit f6a466c into master Jun 4, 2024
30 of 33 checks passed
@alexgrozav alexgrozav deleted the canvas-v2-remove-node-and-connections branch June 4, 2024 12:36
@github-actions github-actions bot mentioned this pull request Jun 5, 2024
Merged
@janober
Copy link
Member

janober commented Jun 5, 2024

Got released with n8n@1.45.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OlegIvaniv OlegIvaniv OlegIvaniv approved these changes

@MiloradFilipovic MiloradFilipovic MiloradFilipovic approved these changes

Assignees

@alexgrozav alexgrozav

Labels
n8n team Authored by the n8n team Released ui Enhancement in /editor-ui or /design-system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexgrozav @MiloradFilipovic @janober @OlegIvaniv