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

fix(LogTable): implement virtual scrolling #1505

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Antreesy
Copy link
Collaborator

@Antreesy Antreesy commented Feb 4, 2025

Attempt to implement 'virtual scrolling':

  • copied from server/apps/settings/src/components/Users/VirtualList.vue, omitting some redundant logic for logreader

TODO:

  • expanded LogTableRow is taking more vertical space, but doesn't seem to break scroller in general, and atm collapses after new scroll event

@Antreesy Antreesy added this to the Nextcloud 32 milestone Feb 4, 2025
@Antreesy Antreesy requested a review from susnux February 4, 2025 10:31
@Antreesy Antreesy self-assigned this Feb 4, 2025
@Antreesy Antreesy force-pushed the fix/1262/virtual-scroller branch from 99e0518 to 3b34500 Compare March 3, 2025 23:04
@Antreesy Antreesy requested a review from ShGKme March 3, 2025 23:05
@Antreesy Antreesy marked this pull request as ready for review March 3, 2025 23:06
@Antreesy Antreesy changed the title [WIP] fix(LogTable): implement virtual scrolling fix(LogTable): implement virtual scrolling Mar 3, 2025
Copy link

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Implementing virtual scrolling is possible, but it is not something simple. A proper raw implementation would be about 300 - 500 more lines.

We do it in the files list in the Files app and then in the accounts table in settings.

Here the only problem is the table layout.

I'd try:

  1. Check if we can use vue-virtual-scroller + display: contents to make div wrappers "invisible" for the tree in the table.
  2. We can fork vue-virtual-scroller to add missing features like containers tag.
  3. Use useVirtualList from vueuse. It might not work well with its default styles, but with constant height we can easily reuse it with translateY instead of marginTop.
  4. Copy implementation from server

Antreesy added 2 commits March 5, 2025 12:56
- copied from server/apps/settings/src/components/Users/VirtualList.vue, omitting some redundant logic

Signed-off-by: Maksim Sukharev <[email protected]>
Signed-off-by: Maksim Sukharev <[email protected]>
@Antreesy Antreesy force-pushed the fix/1262/virtual-scroller branch from 3b34500 to bcde1f7 Compare March 5, 2025 12:00
@Antreesy
Copy link
Collaborator Author

Antreesy commented Mar 5, 2025

Copy implementation from server

Accounts table implementation looks more fitting, so I take over logic from it. Probably need some more style adjustments, but atm looks decent and works fine performance-wise.

cc @Pytal for input as author of original commit

@Antreesy Antreesy requested a review from Pytal March 5, 2025 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants