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

Add reverse support #400

Closed
wants to merge 4 commits into from
Closed

Conversation

bradley
Copy link

@bradley bradley commented Oct 5, 2022

@bradley
Copy link
Author

bradley commented Oct 5, 2022

Im unclear why the package.json changes are here. Isnt 18 the latest?

shouldScroll = instance.options.reverse ? scrollY - prevY : prevY - scrollY;
}

if (shouldScroll) {

Choose a reason for hiding this comment

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

Should this be a more clear boolean comparison, like if (shouldScroll) > 0 ?

Choose a reason for hiding this comment

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

I don't think it should, it seems that the idea is to scroll in either direction if needed.

@@ -341,7 +355,7 @@ export class Virtualizer<TScrollElement = unknown, TItemElement = unknown> {

this.unsubs.push(
this.options.observeElementOffset(this, (offset) => {
this.scrollOffset = offset
this.scrollOffset = offset;

Choose a reason for hiding this comment

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

did you mean to add this ;?

Seems like the use of ; is pretty inconsistent throughout

let end
if (reverse) {
end = measurements[i - 1]
? measurements[i - 1]!.start

Choose a reason for hiding this comment

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

Generally feel a bit hesitant about using !.. is there another way we can write this? Maybe wrap with an if (measurements[i -1]) and then add some kind of error case?

getTotalSize = () => {
let size
if (this.options.reverse) {
size = ((this.getMeasurements()[this.options.count - 1]?.start * -1) ||

Choose a reason for hiding this comment

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

why times -1 here?

Comment on lines +647 to 648
size = ((this.getMeasurements()[this.options.count - 1]?.start * -1) ||
this.options.paddingStart) + this.options.paddingEnd
Copy link

@andremonteiro95 andremonteiro95 Oct 20, 2022

Choose a reason for hiding this comment

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

This won't compile because you may be calculating undefined * -1. See below:

Suggested change
size = ((this.getMeasurements()[this.options.count - 1]?.start * -1) ||
this.options.paddingStart) + this.options.paddingEnd
const measurements = this.getMeasurements()[this.options.count - 1]
size = (measurements ? (measurements.start * -1) : this.options.paddingStart) + this.options.paddingEnd

Also, should you switch paddingStart and paddingEnd since they are reversed?

@Bessonov
Copy link

Hey @bradley do you use virtual for a chat application? If yes, does it work well? I'm looking for a chat/list component and I have tried many components, but no one works well with dynamic elements, which could be appended to top or/and bottom.

@abdullaev388
Copy link

Looks quite good. Will it be merged?

@predam
Copy link

predam commented Dec 21, 2022

@bradley, can you resolve the conflicts?

@jonathanbutler7
Copy link

@bradley this seems like a valuable change, and @lexahall approved it. Any plans to get it merged?

@piecyk
Copy link
Collaborator

piecyk commented Jan 14, 2023

Hi, had a look what we can do on reverse support using current impl, interesting part is the sync scroll when we prepend items on top and don't unmount current items. Got something like this https://codesandbox.io/s/lively-leaf-4mext1?file=/pages/index.js

Basic the idea is to return prev items till the scroll will sync. Question is, when to reset the value that controls rendering prev items 🤔

@jonathanbutler7
Copy link

Hi, had a look what we can do on reverse support using current impl, interesting part is the sync scroll when we prepend items on top and don't unmount current items. Got something like this https://codesandbox.io/s/lively-leaf-4mext1?file=/pages/index.js

Basic the idea is to return prev items till the scroll will sync. Question is, when to reset the value that controls rendering prev items 🤔

@piecyk your codesandbox looks great. What do you mean when you say "scroll will sync"?

@piecyk
Copy link
Collaborator

piecyk commented Jan 16, 2023

What do you mean when you say "scroll will sync"?

@jonathanbutler7 when prepending items we need to preserve current items, this basic means that we need to adjust current scroll position by elements that was added. Also till the scroll happens we need to render same items, that's the pendingSyncScrollRef from the example.

I would go more into that direction, making the core more flexible rather adding more complexity 🤔

@predam
Copy link

predam commented Jan 17, 2023

Hi @piecyk, I appreciate you taking the initiative to make the core more adaptable and providing a sandbox example. This feature is not included by default in other packages, so adding support for it natively rather than requiring users to implement it will be very well received and may encourage more people to adopt the @tanstack/react-virtual library.

@piecyk
Copy link
Collaborator

piecyk commented Jan 17, 2023

Hi @piecyk, I appreciate you taking the initiative to make the core more adaptable and providing a sandbox example. This feature is not included by default in other packages, so adding support for it natively rather than requiring users to implement it will be very well received and may encourage more people to adopt the @tanstack/react-virtual library.

Kinda looks like we already have native support, the trick is to update scroll offset in same time when new items are prepend

https://codesandbox.io/s/beautiful-meninsky-fr6csu?file=/pages/index.js

@jonathanbutler7
Copy link

Hi @piecyk, I appreciate you taking the initiative to make the core more adaptable and providing a sandbox example. This feature is not included by default in other packages, so adding support for it natively rather than requiring users to implement it will be very well received and may encourage more people to adopt the @tanstack/react-virtual library.

Kinda looks like we already have native support, the trick is to update scroll offset in same time when new items are prepend

https://codesandbox.io/s/beautiful-meninsky-fr6csu?file=/pages/index.js

@predam @piecyk you both make good points. Since the "chat" use case is prevalent, it definitely seems like a big value-add for this library to support it in one way or another. It seems that there is a) desire from the user base and b) support from the maintainers.

I see two possible approaches:

  1. Support the use case in the form of a reverse prop (or something similar) as @bradley has proved out in this PR.
  2. Accept @piecyk's codesandbox example as the recommendation, and add documentation on https://tanstack.com/virtual/v3 for implementation details to solve the use case with "native" support

What do you guys think?

@piecyk
Copy link
Collaborator

piecyk commented Jan 18, 2023

Hi @piecyk, I appreciate you taking the initiative to make the core more adaptable and providing a sandbox example. This feature is not included by default in other packages, so adding support for it natively rather than requiring users to implement it will be very well received and may encourage more people to adopt the @tanstack/react-virtual library.

Kinda looks like we already have native support, the trick is to update scroll offset in same time when new items are prepend
https://codesandbox.io/s/beautiful-meninsky-fr6csu?file=/pages/index.js

@predam @piecyk you both make good points. Since the "chat" use case is prevalent, it definitely seems like a big value-add for this library to support it in one way or another. It seems that there is a) desire from the user base and b) support from the maintainers.

I see two possible approaches:

  1. Support the use case in the form of a reverse prop (or something similar) as @bradley has proved out in this PR.
  2. Accept @piecyk's codesandbox example as the recommendation, and add documentation on https://tanstack.com/virtual/v3 for implementation details to solve the use case with "native" support

What do you guys think?

Yes, that's are valid points 👍 Thanks @jonathanbutler7 to be part of the discusion 👍 Just to be clear, i'm not against adding reverse props, just wondering if we really need it.

Overall by design virtual should be low level implementation, allowing to build users any ui on top of if. Looking on @bradley example users still need to add specific css to support the reverse props. Next question how reverse will work when some would like to support adding elements in both directions 🤔

Comparing it to latest example, to support reverse infinite scroll, user need to revers the index and in case when items are prepend sync scroll.

@lexahall
Copy link

lexahall commented Jan 18, 2023

Accept @piecyk's codesandbox example as the recommendation, and add documentation on https://tanstack.com/virtual/v3 for implementation details to solve the use case with "native" support

I would love some documentation added for this! I've been playing with the sandbox today and I get the general idea, but feel foggy on a few things. So far I have not been able to successfully apply this approach to my own codebase.

Next question how reverse will work when some would like to support adding elements in both directions 🤔

Yep, that's the exact issue I'm facing. With chat you need to be able to prepend the chat history as a user scrolls up, but still be able to append new messages.

Thanks so much for looking into this, btw, @piecyk

@Bessonov
Copy link

From my point of view, reverse isn't a solution, it is a workaround, which will work only for a few use cases. In the core, you need add content on top (user scrolls up, possible infinitely) and you will add content to bottom (for instance, new messages arrives). Optionally, it would be great to specify the behavior if not enough content is shown - are messages shown at the top (usually a list) or at the bottom (usually a chat).

@piecyk
Copy link
Collaborator

piecyk commented Jan 18, 2023

From my point of view, reverse isn't a solution, it is a workaround, which will work only for a few use cases. In the core, you need add content on top (user scrolls up, possible infinitely) and you will add content to bottom (for instance, new messages arrives). Optionally, it would be great to specify the behavior if not enough content is shown - are messages shown at the top (usually a list) or at the bottom (usually a chat).

In the end to have bi-directional infinite list we just need to sync scroll in case of prepend https://codesandbox.io/s/infinite-scroll-both-c08vxk?file=/pages/index.js and that's all.

@lexahall
Copy link

@piecyk some interesting thoughts to keep in mind when thinking of something like chat:

  • On initial load, you need to scroll directly to the most recently sent message, generally at the very bottom of the list
  • As new messages are sent, you need to scroll to the end of each message, staying sticky at the bottom
  • Each message can be dynamically sized, so we can't rely too heavily on size estimations

Most of the issues I'm having are around scrolling properly to the last item, either initially or as new messages come in, due to the variable size of the items.

@piecyk
Copy link
Collaborator

piecyk commented Jan 18, 2023

@piecyk some interesting thoughts to keep in mind when thinking of something like chat:

  • On initial load, you need to scroll directly to the most recently sent message, generally at the very bottom of the list
  • As new messages are sent, you need to scroll to the end of each message, staying sticky at the bottom
  • Each message can be dynamically sized, so we can't rely too heavily on size estimations

Most of the issues I'm having are around scrolling properly to the last item, either initially or as new messages come in, due to the variable size of the items.

@lexahall let's add an example to the repo, can you share some example of chat app that we can work on?

@Bessonov
Copy link

@lexahall

As new messages are sent, you need to scroll to the end of each message, staying sticky at the bottom

In my experience, not unconditionally. It depends on where the user is. If the user is a little bit above (iirc we used around 200px), then you will usually show "new message" label instead of direct scrolling. Probably, it need an imperative way to scroll to the specific message with top or bottom alignment.

@lexahall
Copy link

lexahall commented Jan 19, 2023

can you share some example of chat app that we can work on?

@piecyk, like a codesandbox example?

@lexahall
Copy link

@piecyk I'll work on a sandbox, but I have a couple questions about your implementation I want to make sure I understand. I see that you're adjusting the scrollOffset when new items are fetched using:

const nextOffset =
virtualizerRef.current.scrollOffset + pageSize * itemSize;

virtualizerRef.current.scrollOffset = nextOffset;
virtualizerRef.current.calculateRange();

virtualizerRef.current.scrollToOffset(nextOffset, { align: "start" });

For the pageSize * itemSize calculation, this seems very dependent upone knowing the item size. Or at least having a very good estimate. What about cases where the items can be very different sizes? Wont this cause scroll issues?

What does virtualizerRef.current.calculateRange(); do?

@piecyk
Copy link
Collaborator

piecyk commented Jan 19, 2023

@piecyk I'll work on a sandbox, but I have a couple questions about your implementation I want to make sure I understand. I see that you're adjusting the scrollOffset when new items are fetched using:

const nextOffset =
virtualizerRef.current.scrollOffset + pageSize * itemSize;

virtualizerRef.current.scrollOffset = nextOffset;
virtualizerRef.current.calculateRange();

virtualizerRef.current.scrollToOffset(nextOffset, { align: "start" });

For the pageSize * itemSize calculation, this seems very dependent upone knowing the item size. Or at least having a very good estimate. What about cases where the items can be very different sizes? Wont this cause scroll issues?

What does virtualizerRef.current.calculateRange(); do?

@lexahall let's move with discussions to #477

@tannerlinsley
Copy link
Collaborator

This PR is outdated and probably needs a fresh perspective.

@agnauck
Copy link

agnauck commented Jan 1, 2024

Does anyone have a Vue sample for the reverse infinite scroll feature?

@lucassilvas1
Copy link

Does anyone have a Vue sample for the reverse infinite scroll feature?

I'd love one in Svelte as well

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.