fix: prevent returning items when container size is zero

[piecyk]

closes #596

[wuarmin]

Hey @piecyk ,
since this change, the following code no longer works for me.

  useEffect(() => {
    if (data.length === 0) {
      return;
    }

    const firstSelectedRow = Object.keys(cache.getSelectedRows())[0];
    if (firstSelectedRow) {
      rowVirtualizer.scrollToIndex(firstSelectedRow, {
        align: "center",
        smoothScroll: false,
      });
    }
  }, [data, cache, rowVirtualizer]);

The code is in my table component and ensures that the last selected row is visible for the user. This worked fine with v3.0.0-beta.61, and no longer works with version with v3.0.0-beta.61. UseEffect is called as expected, but scrolling does not take place.

Can you please help me?

thanks

[piecyk]

@wuarmin to keep selected rows rendered, please use range extractor

import { defaultRangeExtractor, useVirtualizer } from '@tanstack/react-virtual'

const Foo = () => {
  const selectedIndexes = [0, 50]

  const virtualizer = useVirtualizer({
    count: 100,
    getScrollElement: () => ref.current,
    estimateSize: () => 50,
    rangeExtractor: React.useCallback((range: Range) => {
      if (selectedIndexes.length === 0) {
        return defaultRangeExtractor(range)
      } else {
        const next = new Set([...selectedIndexes, ...defaultRangeExtractor(range)])
        return [...next].sort((a, b) => a - b)
      }
    }, [selectedIndexes]),
  })
  
  return ...
}

[wuarmin]

@piecyk Thanks, I have tried this, but the behavior is not as expected. I just want to keep the scrollbar state when user switch between pages. The table should be scrolled so that the selected row is visible.

The useEffect code did this flawlessly. Why does this no longer work?

Thanks

[piecyk]

@wuarmin ooo that is not good, could you add an issue and create codesandbox example? This need to be fixed before v3 release.

[piecyk]

@wuarmin I think i see the issue, for now you can wrap the scrollToIndex with requestAnimationFrame

requestAnimationFrame(() => {
  virtualizer.scrollToIndex(100)
})

[wuarmin]

@piecyk Thank you very much! That works. Could you please briefly explain, why requestAnimationFrame is necessary?

Thanks

[piecyk]

It's for couple of reasons, mainly the scroll to index when we have dynamic case is not trivial.

When jumping to index after items are mounted we need to read actual size compare it with estimated and update the list if needed, then the position of the index will change, long story short on lib side we do it in loop ( pushing to next tick, so the browser will have time to render items and we can read the size ) it's enabled when we detect dynamic mode.

Basic changes showed that the how we detect dynamic mode is not to robust ( we check if we have the elements in cache )

Could you please briefly explain, why requestAnimationFrame is necessary?

to push the scrolling to next tick, when we have the elements rendered.

[FBNitro]

I have this working on my application, I just had to add the ?? 0 to the rowVirtualizer.getVirtualItems()[0]?.start ?? 0 line, similar to what was done on the PR update to the samples.

But this change made testing with react-testing-library much more difficult because it doesn't actually render in a browser. The actual size of the containers is 0. If you have any suggestions @piecyk please let me know. Options like using cypress are not available to me. I'll try a few things, but open to suggestions. It would be like similar to writing a unit test for the https://tanstack.com/virtual/v3/docs/examples/react/dynamic list example.

[piecyk]

I have this working on my application, I just had to add the ?? 0 to the rowVirtualizer.getVirtualItems()[0]?.start ?? 0 line, similar to what was done on the PR update to the samples.

But this change made testing with react-testing-library much more difficult because it doesn't actually render in a browser. The actual size of the containers is 0. If you have any suggestions @piecyk please let me know. Options like using cypress are not available to me. I'll try a few things, but open to suggestions. It would be like similar to writing a unit test for the https://tanstack.com/virtual/v3/docs/examples/react/dynamic list example.

@FBNitro that is true, for unit test you can mock the getBoundingClientRect for the scroller element, or provide observeElementRect when testing.

[FBNitro]

Ah great, thank you for guiding me in the correct direction. I updated the getBoundingClientRect in the htmlElement prototype and that worked for me.

const DEFAULT_BOUNDING_CLIENT_RECT = window.HTMLElement.prototype.getBoundingClientRect;

beforeAll(() => {
    window.HTMLElement.prototype.getBoundingClientRect = () =>
        ({
            height: 800,
            width: 500
        }) as DOMRect;
});

afterAll(() => {
    window.HTMLElement.prototype.getBoundingClientRect = DEFAULT_BOUNDING_CLIENT_RECT;
});

[brc-dd]

Shouldn't overscan still be respected though? Something like this would've been better:

rangeExtractor({
  ...range,
  overscan,
  count: outerSize === 0 ? 0 : count,
})

[piecyk]

Shouldn't overscan still be respected though? Something like this would've been better:

rangeExtractor({
  ...range,
  overscan,
  count: outerSize === 0 ? 0 : count,
})

don't think so, wouldn't it be counterintuitive that virtualizer returns items here

[brc-dd]

I'm probably wrong, but I always thought of overscan as some sort of guarantee that at least that much rows will be rendered/returned. The docs define overscan as "The number of items to render above and below the visible area.' Here if the container size is 0, the visible area will be 0. Technically 0 + say 10 overscan should still be 10. But yeah I guess this is a bit up to how one is interpreting things.

[piecyk]

Yeah, for example, do we still have visible area if it's 0, also if virtualizer returns range of type { startIndex: number; endIndex: number } what it should return when size it's 0? This is used in rangeExtractor to calculate indexes.

[brc-dd]

Ah, yeah. I guess this is fine. I'll create a separate issue if something goes south with this 👍