Skip to content

virtual Improvements (Dynamic Item Sizes, Fix Broken Reactivity) #803

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jaydevelopsstuff
Copy link

@jaydevelopsstuff jaydevelopsstuff commented Jul 1, 2025

This adds support for dynamic item sizes in the virtual package.

This is accomplished by adding | (row: T[number], index: number) => number) to the rowHeight parameter, and type-checking for either case within createVirtualList.

type VirtualListConfig<T extends readonly any[]> = {
    // ...
    rowHeight: MaybeAccessor<number | ((row: T[number], index: number) => number)>;
};

I also added scrollToItem to VirtualListReturn as a utility function for scrolling to a specified item index.

I'm implementing this because I need it for use in another project, so if I end up needing more features I'll implement them (but I'm also happy to work on other stuff if it's requested). This is an early stage PR and my first crack back at SolidJS in almost half a year after working on other projects, so I'm very rusty and probably didn't implement all of this idiomatically—please correct me if necessary.

(Also contains a small patch to add type to an import in site/src/primitives/DocumentHydrationHelper.tsx, since the dev server would not run without that change)

Copy link

changeset-bot bot commented Jul 1, 2025

⚠️ No Changeset found

Latest commit: 9dc934b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jaydevelopsstuff
Copy link
Author

jaydevelopsstuff commented Jul 1, 2025

I tried to provide a way to access scrollToItem from the VirtualList component, but after doing a bit of searching I couldn't seem to find any documentation on idiomatic ways to do this. So I implemented this wildly hacky method which I'm hoping someone can help me replace with something more practical:

type VirtualListProps<T extends readonly any[], U extends JSX.Element> = {
    // ...
    setScrollToItem: (scrollToItem: (itemIndex: number) => void) => void;
};

// Inside `VirtualList`
let scrollContainer!: HTMLDivElement; // Attached Ref
props.setScrollToItem((itemIndex: number) => scrollToItem(itemIndex, scrollContainer));

In Use:

let scrollToItem!: (itemIndex: number) => void;
// ...
<VirtualList
    // ...
    setScrollToItem={fn => (scrollToItem = fn)}
    // ...
>
{/* ... */}
</VirtualList>

Broken Reactivity *Now Fixed by this PR*

Also, reactivity seems to be broken for the VirtualList component. On the demo site (both in dev and at the main site, changes to # of rows, root height, row height, etc., will be reflected everywhere but inside the component. I fiddled around with things for awhile in an attempt to fix this to no avail.

@jaydevelopsstuff
Copy link
Author

jaydevelopsstuff commented Jul 1, 2025

Alright, I fixed the issue with reactivity being broken by not unwrapping accessors at the top level of createVirtualList.

Would still like some help with exposing scrollToItem properly outside of the VirtualList component though.

@jaydevelopsstuff jaydevelopsstuff changed the title virtual Improvements (Dynamic Item Sizes) virtual Improvements (Dynamic Item Sizes, Fix Broken Reactivity) Jul 1, 2025
overscanCount?: MaybeAccessor<number>;
rowHeight: MaybeAccessor<number | ((row: T[number], index: number) => number)>;
Copy link
Member

Choose a reason for hiding this comment

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

An accessor that returns a function is a bit stupid. Shouldn't it be MaybeAccessor<number> | ((row: T[number], index: number) => number)?

});
});

// Binary Search for performance
Copy link
Member

Choose a reason for hiding this comment

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

The whole function is already pretty complex without the binary search being included inside. I would like to ask you to extract from the function and use offsets as another parameter.

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.

2 participants