-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
fix: Ensure NDV params don't get cut off early and scrolled to the top #11252
base: master
Are you sure you want to change the base?
Conversation
@@ -209,7 +209,7 @@ export const useExpressionEditor = ({ | |||
if (editor.value) { | |||
editor.value.destroy(); | |||
} | |||
editor.value = new EditorView({ parent, state, scrollTo: EditorView.scrollIntoView(0) }); | |||
editor.value = new EditorView({ parent, state }); |
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 scrollIntoView
was added here to fix another NDV issue.
Are you certain that we don't need this anymore?
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.
Thanks for checking this.
I am sure. I also traced it back. Found the original issue it was fixing here
https://www.notion.so/n8n/Other-expressions-initiatives-00fb99a8b49c422fbc2843b5753a6ebb?pvs=4#76fdae74d7f04fa5b822cab6e754c3ae
I tested it and it does not seem needed any more. As such, I added a test for it.
https://github.com/n8n-io/n8n/pull/11252/files#diff-2a8a05f2863b2aae20f63d81e0a01c8f26a7a85b180471b0ceeb80088ddf375aR64
|
n8n Run #7380
Run Properties:
|
Project |
n8n
|
Run status |
Failed #7380
|
Run duration | 04m 03s |
Commit |
0725ce360f: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 mutdmour 🗃️ e2e/*
|
Committer | Mutasem Aldmour |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
1
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
247
|
Tests for review
6-code-node.cy.ts • 1 failed test
Test | Artifacts | |
---|---|---|
... > should show error based on status code |
Test Replay
Screenshots
Video
|
The first 5 failed specs are shown, see all 48 specs in Cypress Cloud.
|
Summary
fixes:
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-2350/bug-ndv-loads-with-params-pane-scrolled-to-the-middle
https://linear.app/n8n/issue/ADO-2595/ndv-parameters-cutting-off-too-early
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)