Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to react-virtual 3.0.1 #348

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

iFreilicht
Copy link

@iFreilicht iFreilicht commented Dec 10, 2023

react-virtual is now available as v3 and published under @tanstack/react-virtual. The old version will not receive updates anymore, see cloudscape-design/components#1765 (comment).

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.

Copy link

vercel bot commented Dec 10, 2023

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
Copy link

@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!

image

@iFreilicht
Copy link
Author

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
Copy link

@timc1 we need this to be merged

Copy link
Owner

@timc1 timc1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@iFreilicht
Copy link
Author

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.

react-virtual is now available as v3 and published under
@tanstack/react-virtual. The old version will not receive updates
anymore, see cloudscape-design/components#1765 (comment).

Fixes timc1#330
@pyyding
Copy link

pyyding commented Mar 6, 2024

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
Copy link
Author

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
Copy link
Owner

timc1 commented Mar 6, 2024

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

@pyyding
Copy link

pyyding commented Mar 7, 2024

@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.

kbar-scroll-demo.mov

@timc1
Copy link
Owner

timc1 commented Mar 7, 2024

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

@iFreilicht
Copy link
Author

@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
Copy link

pyyding commented Apr 10, 2024

image

@timc1 yeah it was only in dev

@clementAC
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency error with react-virtual
6 participants