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

Add virtual scroller to library view #119

Merged
merged 9 commits into from
Oct 1, 2020
Merged

Add virtual scroller to library view #119

merged 9 commits into from
Oct 1, 2020

Conversation

heyhippari
Copy link
Contributor

@heyhippari heyhippari commented Sep 25, 2020

  • Adds a responsive virtual scroller to the library pages
  • Fixes performance issues with the card component

To get around the limitations of the virtual scroller not supporting grids, we use lodash's chunk function to transform the items array with chunks of items (the length of which depends on the breakpoint we're at).

This essentially gives us a bunch of rows of items to pass to the virtual scroll, simulating a grid.

The current implementation of BlurhashImage was somewhat slow in this scenario, so I switched the library to use blurhash-wasm, which is a Web Assembly decoder in Rust (@sparky8251 will be happy 😄 ).
Further performance issues lead to a profiling of the view and I discovered that v-img is quite slow in this scenario. I replaced it with img, since we're not using the features of v-img in this case, which gives us a significant performance boost.

@heyhippari heyhippari mentioned this pull request Sep 25, 2020
90 tasks
v-img appears to be very slow, which makes the rendering of the cards sluggish. This replaces it
with a normal img and uses the transition group to perform the fade in, making the component a lot
faster.
@heyhippari heyhippari marked this pull request as ready for review September 26, 2020 02:17
Maxr1998
Maxr1998 previously approved these changes Sep 26, 2020
@KristupasSavickas
Copy link
Contributor

Would it be possible to preserve the scroll location when navigating backwards? This is a point that many virtual strollers implement wrong and for large libraries it really makes it painful and unintuitive to use.

@KristupasSavickas
Copy link
Contributor

Also, I think more items should be prefetched as there can be noticeable popping while scrolling:

output

The virtual scroller now uses the windows's height times 1.5 as the size of the buffer. This loads
13 rows of 8 items on a 1920x1080 screen, further reducing pop-in when scrolling fast. The
multiplier is expected to be adjusted through testing on more devices down the line.
@heyhippari
Copy link
Contributor Author

heyhippari commented Sep 26, 2020

Would it be possible to preserve the scroll location when navigating backwards? This is a point that many virtual strollers implement wrong and for large libraries it really makes it painful and unintuitive to use.

This should be possible, but there are some considerations:

  • We need to think about the context in which we want scroll to be restored. It doesn't always make sense.
    If we're going back from another page, it makes sense to restore it. If we refresh the page, it makes sense as well. However, if we're coming from somewhere else or this is the first page we see, not having the default state is confusing.
    Vue-router has a "scrollBehavior" for that. Nuxt has a default available here, which is already enabled.
  • We need a bunch of Vuex stores to keep the data cached. As they are now, the states of the pages are lost when navigating away. We want to potentially save the filters and sorting on the pages, so it'd make sense to store the items on a page. It's good to not have wait time for the user as well. We'd update items when navigating, but existing items would be shown. This is a PR on it's own, perhaps even multiple ones.

Also, I think more items should be prefetched as there can be noticeable pop-in while scrolling

This definitely needs some improvement, for sure. It's probably something we'll be adjusting and optimizing as we go along.

All the items in the library are actually pre-fetched, we don't do any lazy-loading. What's happening here is that the virtual scroller is having a hard time keeping the cards updated with your scrolling.
The way it works is that we have a fixed number of rows rendered at a time. We have the ones shown on screen, plus a buffer on top and on the bottom of the screen.
Currently, on a 1080p screen with the window maximized, this amounts to 8 lines of 8 items being rendered at a time, so 64 cards.
Once you scroll, whenever the row goes past the buffer, it is moved down to below the current scroll position and updated with the next chunk of items.

Do note that running this in dev mode (yarn dev) makes the client significantly slower than running it in production (yarn build && yarn start or yarn build and hosting it on an SPA-compatible server). This is due to most things having a lot of debugging facilities that the current jf-web doesn't have, as well as a ton of optimizations not being applied, in order to make hot-reloading faster.
When running in production mode, there is pretty much no pop-in when going at a reasonable speed (What the average person would browse at when looking for something to watch). Of course, when jumping around the page or going very fast, you're going to have some unloaded items show up and some card pop-in. That's pretty much inevitable, with this way of doing things.

I have adjusted the buffer to be reactive with the window height and load more items, which now brings the total of rendered rows to 13, so 104 items. This should alleviate the issue for now, but keep in mind it'll be a balancing act for a while, as we test this on more and more devices.

During the move to BlurhashImage, a CSS property for fitting the image properly was lost. This
restores it, fixing the aspect ratio issues on card images.
@sonarcloud
Copy link

sonarcloud bot commented Sep 26, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@camc314 camc314 merged commit 93fe014 into master Oct 1, 2020
@camc314 camc314 deleted the feat/improvements branch October 1, 2020 09:19
@heyhippari heyhippari added this to the Preview Release 1 milestone Dec 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants