-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
perf(editor): Use virtual scrolling in RunDataJson.vue
#10838
perf(editor): Use virtual scrolling in RunDataJson.vue
#10838
Conversation
RunDataJson.vue
RunDataJson.vue
RunDataJson.vue
const jsonDataContainer = this.$refs.jsonDataContainer as HTMLDivElement; | ||
this.jsonDataContainerHeight = jsonDataContainer.offsetHeight; |
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.
have you considered using https://vueuse.org/core/useElementSize/#usage ?
this implementation won't work properly if for some reason the user resizes the screen
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.
Ahh interesting! Will update it to use it. Thanks for the suggestion. Did not know about it. As you can see, I'm a newbie in the FE.
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.
Agree with @r00gm, either that or with manual listener but we need to make sure it's updated when the element is resized.
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.
Will apply this PR is merged, as could not get the reference to the div in the template from the setup method. Probably because we are mixing setup method and options API.
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.
You should be able, you just need to return it from the setup
method but still nice that you did the migration.
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.
Added 2 comments
const jsonDataContainer = this.$refs.jsonDataContainer as HTMLDivElement; | ||
this.jsonDataContainerHeight = jsonDataContainer.offsetHeight; |
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.
Agree with @r00gm, either that or with manual listener but we need to make sure it's updated when the element is resized.
n8n Run #6954
Run Properties:
|
Project |
n8n
|
Branch Review |
ado-2598-enable-virtual-scrolling-in-node-output
|
Run status |
Passed #6954
|
Run duration | 04m 42s |
Commit |
69b50eb27d: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 RicardoE105 🗃️ e2e/*
|
Committer | Ricardo Espinoza |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
2
|
Pending |
0
|
Skipped |
0
|
Passing |
430
|
View all changes introduced in this branch ↗︎ |
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.
Nice. Looks good.
✅ All Cypress E2E specs passed |
Got released with |
Summary
Community member suggested to enable virtual scrolling in
vue-json-pretty
to improve performance when rendering big datasets. There is a massive difference in performance when virtual is enabled.Before: https://share.cleanshot.com/MF4hyWMN
Now: https://share.cleanshot.com/G2v0kSPg
Related Linear tickets, Github issues, and Community forum posts
GH issue: #7584
Linear ticket: https://linear.app/n8n/issue/ADO-2598/enable-virtual-scrolling-in-node-output
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)