Upgrade to react-virtual 3.0.1

[iFreilicht]

react-virtual is now available as v3 and published under @tanstack/react-virtual. The old version will not receive updates anymore, see https://github.com/cloudscape-design/components/pull/1765#issuecomment-1839426894.

Fixes #330

This required a few more changes than I initially anticipated, but from what I can tell this works perfectly now.

There is a new estimateSize function, react-virtual v3 requires specifying that even if only dynamically-sized elements are used. The docs recommend:

[...] estimate the largest possible size (width/height, within comfort) of your items. This will ensure features like smooth-scrolling will have a better chance at working correctly.

AFAICT, this shouldn't matter for kbar and I don't think it makes sense to expose this value as a configurable prop.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
kbar	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 2, 2024 1:35pm

[Haarolean]

@iFreilicht hey, just curious: since the author is inactive I wanted to fork the repo and apply your changes. Am I doing something wrong here, or, perhaps, it's unfinished? Thanks!

[iFreilicht]

Hmmm, I'm not really sure, but my assumption is that the dependency @types/react is to blame. If you take a look at the file example/src/App.tsx in my branch, you'll see that it also uses KBarProvider, but no error like the one you're showing is thrown. In this project's package-json.lock, @types/react is version 17.0.64. Maybe in your project it is higher or lower, and there is some incompatible difference?

From my testing, this change is ready, but I didn't actually use it in a project after all. I think I wanted to use spectacle with node 21 or something, and this was the blocker, but in the end I just decided to use node 20.

[ketangupta34]

@timc1 we need this to be merged

[iFreilicht]

Ah thank you, I fixed this now! Unfortunately, it seems scrollToIndex is not as robust anymore as it was, it doesn't work when the scroll element is 0px high. I had to implement a bit of an ugly workaround with setTimeout to defer the scrolling action until after the height animation starts. But this does seem to work reliably now.

[pyyding]

There's a slight regression in expected behavior here – when navigating back, the scroll position is incorrect:

Screen.Recording.2024-02-04.at.1.28.16.PM.mov

I have this even without the react-virtual update. I'd say it's a separate ticket

[iFreilicht]

Ah that's good to know! @timc1, if you don't like my workaround, I'll happily revert it so this can be fixed in a separate PR.

[timc1]

@pyyding can you share a screen cap of that behavior with the current version?

[pyyding]

@timc1

Sure! This demo shows it sometimes doesn't open the menu scrolled to top and sometimes when navigating back.

I briefly went over the source code trying to find the bug but it's not as obvious as I'd initially expected. Feel free to share some pointers! Because if it doesn't magically get fixed, I'm gonna have to deepdive into it soonish.

https://github.com/timc1/kbar/assets/16744172/5d300b1f-7cb7-4bfe-83cb-898d8c87072f

[timc1]

@pyyding awesome! And is this during dev or production?

[iFreilicht]

@pyyding The fix to this specific bug is here: https://github.com/timc1/kbar/pull/348/files#diff-e82a08a30f41d6167d79fe54b58c564bbc4d5ddb301aecbf6de659adcc50888dR98-R109

My theory is that the issue is caused by the fact that scrolling the list back to the top and resizing the list so it is visible again are two separate effects, the order of which is undefined. If the resize happens first, scrolling works fine, but if the list is still invisible, scrolling won't work. The fix delays scrolling so it always happens after resizing.

I'm not entirely sure why the scroll position moves in the first place, though. Maybe it would be better to fix that.

[pyyding]

@timc1 yeah it was only in dev

[clementAC]

Although it's a helpful tool, the warning in the installation may not help users adopt the library (users can be afraid of it or even wonder if it is still maintained). As it seems not to have any side effects, is it possible to publish a new version?

[stale[bot]]

Hey! This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[iFreilicht]

Still relevant, do not close

[Haarolean]

Bad bot