tnrich / react-variable-height-infinite-scroller

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

Seems to be a bug with preloadRowStart/jumpToRow #12

Closed dozoisch closed 9 years ago

dozoisch commented 9 years ago

I don't know if you are able to repro on your side.

If I have a total of something like 10 rows, and I want to start near the end (like row 8), it'll actually start at like 8 - minNumberOfRowVisible. (which is often 0).

it seems to be caused by this part in componentDidUpdate

    if (this.rowStart - 1 > 0) {
       this.prepareVisibleRows(this.rowStart - 1, this.state.visibleRows.length + 1); // don't want to just shift view
   } 

if I put a console log in just before the prepareVisibleRows of the rowStart it goes 8, 7, 6, 5, 4, 3, 2, 1, 0, and then start scroll at 0. I haven't figured out a way to fix it yet. My current work around is to add a bunch of rows at the end :/ So it starts at 8 correctly.

If i have a lot more rows, it works as expected

tnrich commented 9 years ago

Hey @dozoisch, I believe this should be by design. In componentDidUpdate, I check to make sure that the visibleRowsContainer is taking up enough space (which means that it fully fills the containerHeight if possible), and if it isn't, I either add more rows to the bottom or the top. Here's the code in question:

// check if the visible rows fill up the viewport 
    // tnrtodo: maybe put logic in here to reshrink the number of rows to display... maybe...
    if (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) {
          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) {
          this.prepareVisibleRows(0, this.state.visibleRows.length + 1);
        }
      }
    }

So in your case, you start with just 1 row, row 8, and its positioned at the top of the container. Then it sounds like there are no rows it can add below (it tries to do that first), so it adds a row above, and does the same check. Ideally row 8 should still be fully visible at the end of the loading, and if it isn't, that's a bug.

Does that all make sense?

dozoisch commented 9 years ago

Yeah that makes sense.

In my case the row I asked for gets pushed out of the screen, and we see only rows 0 to 6. (7 to 10 being pushed out of bottom of the screen). Don't know if this is expected.

Ill try to figure out if this is something I'm configuring wrongly.

I also tried doing a setTimeout and setting the rowToJumpTo in my component did mount (after 500ms) and it didn't jump to row 8. (stayed a 0)

tnrich commented 9 years ago

Hmm, it sounds like this is indeed a bug. I doubt you're configuring something wrong. This seems like it might be a good time to write a test case.

tnrich commented 9 years ago

Hey @dozoisch, do you have any experience testing react components? I tried pretty unsuccessfully just now to get some unit tests up and running for the component. I ran into jsdom node incompatibility issues mainly. And then even when I though I got an older version of jsdom up and running, I still was getting hit with this error:

Error: Invariant Violation: dangerouslyRenderMarkup(...): Cannot render markup in a worker thread. Make sure `window` and `document` are available globally before requiring React when unit testing or use React.renderToString for server rendering.

Definitely not as straightforward as I thought it would be...

tnrich commented 9 years ago

Okay, I found a testing framework that seems like it should work out. I'm using smokestack to run the tests. I've started to develop it on a tnr_testing branch if you're interested in taking a look at it. I haven't gotten very far with it yet, but it seems like it should do the job.

Also, I think that this could be a good place to draw inspiration from: https://github.com/seatgeek/react-infinite/blob/master/__tests__/infinite_test.js

dozoisch commented 9 years ago

I'll try to take a look! I'm in a bit of a crunch right now! React-infinite seems like a good place to draw inspiration from what I saw of their test suite!

tnrich commented 9 years ago

Cool beans! No pressure to get to it right away.

tnrich commented 9 years ago

Hey @dozoisch , I spent a bit of time today and I think I have a fix for the rendering to the last row (or a row near the bottom in general). Currently that row is placed as high as possible within the visible container. Do you need an option for placing the row as low as possible in the visible container?

I'll push the changes and an example soon. I've also collapsed the preloadRowStart prop into the rowToJump prop.

dozoisch commented 9 years ago

Cool! I'll try it!

On my side I do not need the"as low as possible" option!

tnrich commented 9 years ago

Okay cool. I'll wait to implement that until someone actually needs it. The demo page has the new jump ability displayed in a second example.

On Tue, Oct 27, 2015, 7:30 AM Hugo Dozois notifications@github.com wrote:

Cool! I'll try it!

On my side I do not need the"as low as possible" option!

— Reply to this email directly or view it on GitHub https://github.com/tnrich/react-variable-height-infinite-scroller/issues/12#issuecomment-151517294 .

tnrich commented 9 years ago

I added the "low as possible" option anyways just in case people would want that. I could image a chat app would want to jump to the bottom of the very last row.

dozoisch commented 9 years ago

True that! Good Idea

On Tue, Oct 27, 2015 at 2:24 PM, Thomas Rich notifications@github.com wrote:

I added the "low as possible" option anyways just in case people would want that. I could image a chat app would want to jump to the bottom of the very last row.

— Reply to this email directly or view it on GitHub https://github.com/tnrich/react-variable-height-infinite-scroller/issues/12#issuecomment-151599930 .

tnrich commented 9 years ago

Hey @dozoisch , Want to check that this is working and close this issue?

dozoisch commented 9 years ago

Just tested it, seems to work well!

tnrich commented 9 years ago

@dozoisch thanks! Much appreciated :)