search

tbranyen / hyperlist

A performant virtual scrolling list utility capable of rendering millions of rows
Other
448 stars 45 forks source link

Tweak compute scroll height by using a median #27

Closed soyuka closed 7 years ago

soyuka commented 7 years ago

I noted that when you have really different heights, there is a possibility that _screenItemsLen resolves to 1 (then 3) and therefore you might be missing elements at the top/bottom (position start 0 / position end total).

To resolve this issue I've changed the height computation to use a median instead of a basic average. Also, we will keep the highest value of _screenItemsLen to avoid setting it less then required (really depends on the height of the elements currently shown).

This may increase the number of rendered elements, but it has almost no performance overhead, and it's more important to guarantee that all the elements are shown.

tbranyen commented 7 years ago

Excellent, I believe this was one of the points made in https://github.com/tbranyen/hyperlist/pull/20#issuecomment-298444117 wrt missing items. Any thoughts on how a test could be written to account for this behavior?

soyuka commented 7 years ago

It was hard to reproduce but I'll come up with something!

tbranyen commented 7 years ago

@soyuka you can always merge this first and we can add a test later. Don't wanna hold up the code if we don't have to.

soyuka commented 7 years ago

I'm having trouble finding a way to change the rendered elements according to the given scrollTop. You may have some idea for this?

For example:

  1. init
  2. scroll to bottom => test that last element is the correct one
  3. scroll to top => test that the first element is the correct one
soyuka commented 7 years ago

@tbranyen Finally updated with a nice test!

The test prepares 100 items with heights between 100 and 50. It then scrolls with a += minHeight gap until it reaches the end. When the scrollHeight is bigger then the container computed height, we check that it did show every 100 items!

Because this is really random and that I successfully ran this test with crazy values this is :+1:!

Also, I had to increase the mocha timeout. Indeed it takes ~2.3 seconds to scroll 50 by 50 until the end (approx. 7500px - can't say for sure because it's dynamic and random :D).

Feel free to merge!