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

Padding is buggy #272

Closed
mattwondra opened this issue Dec 1, 2023 · 6 comments · Fixed by #303
Closed

Padding is buggy #272

mattwondra opened this issue Dec 1, 2023 · 6 comments · Fixed by #303
Assignees

Comments

@mattwondra
Copy link

Describe the bug
Adding padding to the VList is broken in a couple ways:

1. When scrolling down items at the top of the list can momentarily disappear

  • Open the "Padding and Margin" story
  • Change item height from 100 to 10 so each item is smaller
  • Scroll down the list — as each item nears the top it will disappear, and then reappear when you stop scrolling
  • Adjusting overscan doesn't seem to have any impact
  • Expected: Items that are visible to the user should never disappear

2. Padding messes up scrolling to end of the list

  • Open the "Reverse" story
  • To the VList style add paddingTop: 100
  • Instead of auto-scrolling to the very bottom of the list, it scrolls to near the bottom but not all the way
  • This also happens with paddingBottom
  • This also happens when you remove reverse
  • This also happens when you use scrollTo(99999) for pixel-specific scroll position
  • Expected: scrollTo and scrollToIndex should behave the same whether or not the VList has padding

3. Padding on short list adds unnecessary scrollbars

  • Open the "Reverse" story with a tall browser window
  • To the VList style add paddingTop: 100
  • Instead of 1000 rows just render 10
  • Even though the items + padding are shorter than the container height, scrollbars still show
  • This also happens with paddingBottom
  • This also happens when you remove reverse
  • Expected: When the rows + padding are smaller than the container, there should not be scrollbars

To Reproduce
Listed above.

Expected behavior
Listed above.

Platform:

  • OS: MacOS
  • Browser: Chrome, Firefox, Safari
  • Version of react: 18.2.0
  • Version of this package: 0.17.4
@inokawa
Copy link
Owner

inokawa commented Dec 2, 2023

Padding is always unintentionally buggy. header/footer div approach like other libraries might be better.

@inokawa inokawa self-assigned this Dec 2, 2023
@mattwondra
Copy link
Author

I'm working with a chat-like app (reverse-scroll, infinite load from the top). I was looking at padding to solve two problems:

  1. Give a little more room at the bottom. When scrolled all the way to the bottom, I wanted a little more space between the bottom message and the container, for breathing room. This is simply solved by adding a spacer item to the list.

  2. Show "Loading..." at the top when loading new messages. As noted in the VList Stories, this use case isn't supported. Using a spacer at the top of the list is the same as using the spinner — things get jumpy when new items load and the position of the static top div changes. I thought I could try padding-top and custom components to position a "Loading..." indicator in the right place, outside of the virtualized list items.

So I guess for my purposes, a solution to #163 would give me what I need. In the meantime, I suppose I'll need to find a new place in the UI to indicate loading that isn't inside the list itself.

@inokawa
Copy link
Owner

inokawa commented Dec 5, 2023

Thank you for the information. I'll check if it's possible.

@inokawa
Copy link
Owner

inokawa commented Dec 6, 2023

@mattwondra
I fixed #163 in 0.17.5. It will require adding +1 to index if you put header on top of the list and call scrollToIndex.

I feel that this lib needs stable way to add padding/header/footer. However it may take time because there are several things to consider.

@mattwondra
Copy link
Author

@inokawa Wow, well done fixing #163! These near-daily updates continue to impress me. That completely solves my own immediate need for padding, so no rush on my end to address this issue.

@inokawa
Copy link
Owner

inokawa commented Jan 7, 2024

Incomplete padding support was dropped from VList in 0.20.0. Use new Virtualizer which has header/footer support. It only supports static header/footer. Dynamic ones will be supported in #315 .

Workaround like #272 (comment) works for now but it may be dropped in the future.

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 a pull request may close this issue.

2 participants