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

feat: lit integration #718

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

feat: lit integration #718

wants to merge 14 commits into from

Conversation

kadoshms
Copy link

Lit is a growing library for building web-components with ease.
This PR introduces @tanstack/lit-virtual integration package for creating virtualized elements using Lit's Reactive Controllers

I guess there's more room for improvement and testing, but it could definitely be a start for this.

Copy link

nx-cloud bot commented Apr 29, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d783e40. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


🟥 Failed Commands
nx affected --targets=test:format,test:sherif,test:knip,test:lib,test:types,test:build,build

Sent with 💌 from NxCloud.

@kadoshms
Copy link
Author

kadoshms commented May 1, 2024

@tannerlinsley @piecyk how can we move forward with this? :)

Copy link
Collaborator

@piecyk piecyk left a comment

Choose a reason for hiding this comment

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

@kadoshms awesome work 🥇 Had a quick look on adapter and added two comments, will check the examples letter but overall i think we can merge it soonish 👏

packages/lit-virtual/src/index.ts Outdated Show resolved Hide resolved
Comment on lines 49 to 53
this.cleanup = this.virtualizer._didMount()
this.virtualizer._willUpdate()
await this.host.updateComplete
this.host.requestUpdate()
this.virtualizer.measure()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm why not?

Suggested change
this.cleanup = this.virtualizer._didMount()
this.virtualizer._willUpdate()
await this.host.updateComplete
this.host.requestUpdate()
this.virtualizer.measure()
this.cleanup = this.virtualizer._didMount()
this.virtualizer._willUpdate()

as _willUpdate will set up event listeners and call onChange if needed, there we have requestUpdate, the measure is pretty a hack to clear cache and call onChange

Copy link
Author

Choose a reason for hiding this comment

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

Actually I started with this approach at the beginning, but for some reason in the Columns example, if failed to size the columns right, it would fix only after a scroll event was triggered.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issue here is that for initial measure, when we call measureElement node.isConnected will return false for some reason https://github.com/TanStack/virtual/blob/main/packages/virtual-core/src/index.ts#L648

Copy link
Author

Choose a reason for hiding this comment

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

Yes I see, that's mainly because on the first render the ref will be undefined , and virtual dynamic nodes aren't connected yet, but this "gets fixed" by immediately measuring and invoking a render.
There was indeed a redundant _willUpdate which I removed, but the rest I'm not sure it's possible without touching the core.

WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

Hey @piecyk, it's been a while :) do we plan to move forward with this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kadoshms please checkout #737 wit this change we can simplify the adapter

Nagranie.z.ekranu.2024-06-1.o.13.18.54.mp4

Copy link
Author

Choose a reason for hiding this comment

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

Awesome 👍
I'll check this out merged with my branch and report back this week

Copy link
Author

Choose a reason for hiding this comment

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

@piecyk for column & grid dynamic virutalizers that works great - however I noticed a small glitch with row - if you scroll down to the very bottom, there's some weird behavior:
scroll

Copy link
Author

Choose a reason for hiding this comment

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

It's a bit hard to see it with the gif

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit hard to see it with the gif

hmm can't really reproduce it, on what browser and os you are getting this? Btw changed a bit the impalemntion on #737 to make it simpler added the measureElement back on virtual item.

@kadoshms kadoshms requested a review from piecyk May 1, 2024 13:54
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.

None yet

2 participants