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

Headless virtual list #702

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

Conversation

SpencerWhitehead7
Copy link
Contributor

As previously suggested, this is an attempt at rewriting the virtual list comp to be a headless utility function and adds tests and docs. It still exports the VirtualList comp for convenience, which uses the utility internally. The utility and comp are tested separately.

I think a lot of the TS in here is gross and people who know solid better than me might have better ideas for how to handle the ref. However, it does work and most of the ugliness is hidden from consumers. I don't really like the API, in terms of what you need to pass in and what comes back out and how you have to use it with the JSX, but I couldn't find anything to remove/add/simplify from/to/in it.

Copy link

changeset-bot bot commented Sep 30, 2024

🦋 Changeset detected

Latest commit: 74549d1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@solid-primitives/virtual Minor

Not sure what this means? Click here to learn what changesets are.

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

@SpencerWhitehead7
Copy link
Contributor Author

@thetarnav #667 (comment)

Copy link
Member

@thetarnav thetarnav left a comment

Choose a reason for hiding this comment

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

I like this direction. There has already been an issue about not being able to control some part of rendering: #698 so this will be very helpful, even if a lot of people will end up using the component for convenience.

I don't really like the API

That may be because the boilerplate of inputs and outputs and passing everything makes more code than the calculation itself.

A non-reactive function like this would work as well:

export function getVirtualList(offset, items, rowHeight, overscanCount) {

  let firstIdx = Math.max(0, Math.floor(offset / rowHeight) - overscanCount)
  let lastIdx = Math.min(
    items.length,
    Math.floor(offset / rowHeight) + Math.ceil(rootHeight / rowHeight) + overscanCount,
  )

  return {
    firstIdx,
    lastIdx,
    containerHeight: items.length * rowHeight,
    viewerTop: firstIdx * rowHeight,
    visibleItems: items.slice(firstIdx, lastIdx),
  }
}

Comment on lines +20 to +31
}: {
rootElement: Accessor<Element>;
items: T | undefined | null | false;
rootHeight: number;
rowHeight: number;
overscanCount?: number;
}): {
onScroll: VoidFunction;
containerHeight: () => number;
viewerTop: () => number;
visibleItems: () => readonly T[];
} {
Copy link
Member

Choose a reason for hiding this comment

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

The object literals should be named. Eg VirtualListConfig and VirtualListReturn.

Comment on lines +28 to +30
containerHeight: () => number;
viewerTop: () => number;
visibleItems: () => readonly T[];
Copy link
Member

Choose a reason for hiding this comment

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

The returned functions could be written with Accessor to show that they are reactive.

Comment on lines +22 to +25
items: T | undefined | null | false;
rootHeight: number;
rowHeight: number;
overscanCount?: number;
Copy link
Member

Choose a reason for hiding this comment

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

All of the inputs could be signals. eg with using MaybeAccessor and access from utils or unwrapping manually.

rowHeight,
overscanCount,
}: {
rootElement: Accessor<Element>;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having both rootElement: Accessor<Element> and onScroll: VoidFunction, I think we should stick to one of those:

  • rootElement: Accessor<Element | undefined | null | false> and we add an event listener in an effect ourselves, no need for handling scroll by the user.
  • offset: Accessor<number> and the user instead of creating a signal for the element, created one for the scrollTop value and manages that - it's the same amount of code for the user anyway
  • onScroll: (el: Element) => void and we read the scrollTop from the element and the user adds the listener
  • onScroll: (offset: number) => void and the user reads the scrollTop from the element and adds the listener

Comment on lines +45 to +52
return {
onScroll: () => {
setOffset(rootElement().scrollTop);
},
containerHeight: () => items.length * rowHeight,
viewerTop: () => getFirstIdx() * rowHeight,
visibleItems: () => items.slice(getFirstIdx(), getLastIdx()),
};
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be split into a tuple [read: Accessor<Returns>, write: OnScroll]

Suggested change
return {
onScroll: () => {
setOffset(rootElement().scrollTop);
},
containerHeight: () => items.length * rowHeight,
viewerTop: () => getFirstIdx() * rowHeight,
visibleItems: () => items.slice(getFirstIdx(), getLastIdx()),
};
return [
() => ({
containerHeight: items.length * rowHeight,
viewerTop: getFirstIdx() * rowHeight,
visibleItems: items.slice(getFirstIdx(), getLastIdx()),
}),
() => setOffset(rootElement().scrollTop),
]

},
containerHeight: () => items.length * rowHeight,
viewerTop: () => getFirstIdx() * rowHeight,
visibleItems: () => items.slice(getFirstIdx(), getLastIdx()),
Copy link
Member

Choose a reason for hiding this comment

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

getFirstIdx and getLastIdx could be returned as well

@@ -9,7 +9,8 @@
[![version](https://img.shields.io/npm/v/@solid-primitives/virtual?style=for-the-badge)](https://www.npmjs.com/package/@solid-primitives/virtual)
[![stage](https://img.shields.io/endpoint?style=for-the-badge&url=https%3A%2F%2Fraw.githubusercontent.com%2Fsolidjs-community%2Fsolid-primitives%2Fmain%2Fassets%2Fbadges%2Fstage-0.json)](https://github.com/solidjs-community/solid-primitives#contribution-process)

A basic [virtualized list](https://www.patterns.dev/vanilla/virtual-lists/) component for improving performance when rendering very large lists
A headless `createVirtualList` utility function for [virtualized lists](https://www.patterns.dev/vanilla/virtual-lists/) and a basic, unstyled `VirtualList` component (which uses the utility).
Copy link
Member

Choose a reason for hiding this comment

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

You could make createVirtualList and VirtualList into section headers and add hash links here like in other packages.

@@ -23,18 +24,82 @@ pnpm add @solid-primitives/virtual

## How to use it

`createVirtualList` is a headless utility for constructing your own virtualized list components with maximum flexibility.
Copy link
Member

Choose a reason for hiding this comment

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

createVirtualList should be added to primitives list in package.json

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