tnrich / react-variable-height-infinite-scroller

variable row height scroller (no precomputation of row height necessary)
29 stars 4 forks source link

problem jumping to a row #14

Closed violinchris closed 8 years ago

violinchris commented 8 years ago

First things first, thanks. 'Variable height' was exactly what I was looking for.

Not sure if this is a bug, or if I am doing something wrong. Sometimes the row jump seems to work, sometimes not. I am generating my rows differently than the demo, so I hope that's not related to the problem. Anyway, here is what I think is wrong.

I am running into a situation where the visibleRows height/2 <= containerHeight, and no rows need to be added to the bottom, and no rows need to be added to to top. Should that be possible? I may be misunderstanding, but that doesn't seem to be considered a possibility, yet this seems to be what is happening. I added an else after the else if to show this. When this happens, obviously the row jump doesn't happen.

Here is a paste of the section where I added the else.

    if (visibleRowsContainer.getBoundingClientRect().height / 2 <= this.props.containerHeight) {
      // console.log('visibleRowsContainer.getBoundingClientRect().height / 2 <= this.props.containerHeight');
      // visible rows don't yet fill up the viewport, so we need to add rows
      if (this.rowStart + this.state.visibleRows.length < this.props.totalNumberOfRows) {
        // load another row to the bottom
        this.prepareVisibleRows(this.rowStart, this.state.visibleRows.length + 1);
      } 
      else {
        // there aren't more rows that we can load at the bottom so we load more at the top
        if (this.rowStart - 1 > 0) {
          console.log(this.rowStart);
          this.prepareVisibleRows(this.rowStart - 1, this.state.visibleRows.length + 1); // don't want to just shift view
        } 
        else if (this.state.visibleRows.length < this.props.totalNumberOfRows) {
          console.log('***',this.rowStart);  
          this.prepareVisibleRows(0, this.state.visibleRows.length + 1);
        } 
        else {
          console.log('row jump doesnt happen')
        }
      }
    } 
    else if (this.rowJumpTriggered) {
      console.log('if we ever got here, things turned out right');
tnrich commented 8 years ago

Hey @violinchris, thanks for the bug report. I'll take a look at the code in question. Is there any chance you could try to replicate the error either with the demos in the repo itself, or within your own code?

tnrich commented 8 years ago

Hmm, looking at the logic there, it doesn't seem like that is likely to be the culprit. I believe the only time it would make it to the "else" statement you inserted, would be if the else-if right before it wasn't caught, i.e.:

this.state.visibleRows.length >= this.props.totalNumberOfRows

which would mean that all of the rows are being shown.

In which case we don't want to trigger a prepareVisibleRows() call, or else I think the render cycle could infinitely loop, which is why there is no else there.

I think I might have an idea of what could be going wrong though. I think that the row jump probably only is messing up for "small" amounts of rows, such that all the rows fit fully into the visibleRowContainer and visibleRowsContainer.getBoundingClientRect().height / 2 <= this.props.containerHeight is true. But, they're also large enough so that there is some scrolling allowed, and a row jump would be necessary.

Can you verify that that's the case? If so I think the fix would be to conditionally handle the rowJumpTriggered logic where you've put the extra else as well as in it's current location.

violinchris commented 8 years ago

i just noticed that i never have a problem if the rows are big. if random height of 50-90px, no problem, ever. 10-30, problem almost every time. that is with a containerHeight of 336 and totalNumberOfRows 32.

just noticed your latest comment so i am not going to finish mine, other than to say my average of 20px * 32 / 2 = 320 which is less than the 336 containerHeight. i think what you wrote is correct.

tnrich commented 8 years ago

Okay, I'll make a fix and publish a beta, and then we can test it out.

tnrich commented 8 years ago

I went ahead and released the fix. :) Tell me if it solves your issue. Thanks for reporting it!

violinchris commented 8 years ago

Thanks. Everything seems to work.