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 5 commits into
base: master
Choose a base branch
from

Conversation

Antreesy
Copy link
Collaborator

@Antreesy Antreesy commented Feb 4, 2025

Ref #1262

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

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

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

// Items to render before and after the visible area
const bufferItems = 3
// Fixed height of LogTableRow
const itemHeight = 42

Choose a reason for hiding this comment

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

Absolutely ignore this comment if it's not helpful (I'm not a Vue dev, but I've worked on similar things in Flutter), but this and theadHeight may not be constant if I understand the implementation.

For example, some users may opt to increase the scale of their browser's content on high DPI displays or to aid readability. I can imagine scaling much beyond the assumed heights here would stop scrolling working as intended.

Of course, CSS and other elements may change outside of this file too in future commits.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Although actual amount of screen pixels taken might vary, from CSS perspective it's still the same parameters: font-size: 15px, button height: 34px. Current logic modifies and relies on CSS values, so should be fine.

Adding a resize check in case text wrapping may happen (hope it's not an overhead), but default should be aligned in the code anyway (starting ref(value) and style's min-height)

Antreesy added 4 commits March 6, 2025 17:54
Signed-off-by: Maksim Sukharev <[email protected]>
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 bcde1f7 to c0a3862 Compare March 6, 2025 17:28
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.

3 participants