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

[BUG] Items get calculated incorrectly when child of flex parent #522

Closed
genepaul opened this issue Dec 8, 2021 · 11 comments
Closed

[BUG] Items get calculated incorrectly when child of flex parent #522

genepaul opened this issue Dec 8, 2021 · 11 comments
Labels
bug Something isn't working

Comments

@genepaul
Copy link
Contributor

genepaul commented Dec 8, 2021

Describe the bug
This may be a bug, or it may be incorrect usage. I am using Virtuoso inside a Material UI Popover, and placing it inside a flex container so it takes up the remaining portion of the popover after some content (see codesandbox link). My understanding is I should be able to have a structure like this:

<div style={{
   display: 'flex',
   flexDirection: 'column',
   height: '100%'
  }}
>
  <div> {'some content of unknown or variable height'}</div>
  <div style={{flex: 1}}>
      <Virtuoso height={'100%'} .../>
  </div>
</div>

And it seems this works just fine when this is just in the regular DOM, as shown in the codesandbox here. But when I put it in a Popover with this same sort of structure, it doesn't seem to be able to calculate the right number of items to show, and you'll notice when scrolling to the bottom of the list there's a gap and not all of them render.

Reproduction
https://codesandbox.io/s/vigilant-rgb-xugwm?file=/src/components/App.js

To Reproduce
Steps to reproduce the behavior:

  1. Go to the codesandbox
  2. Click the Expand button
  3. Scroll the list down

Expected behavior
Should render all items with no gap at the bottom

Desktop (please complete the following information):

  • OS: [e.g. iOS] - macOS Big Sur
  • Browser [e.g. chrome, safari] - Chrome
@genepaul genepaul added the bug Something isn't working label Dec 8, 2021
@genepaul
Copy link
Contributor Author

genepaul commented Dec 8, 2021

I will note that the example codesandbox I linked to where it seemed to be working is using version 0.12.0, while my codesandbox is using 2.2.7 (and I've been able to reproduce up to the latest version). But if I update the example (not in a popover) to latest, it still does not reproduce the issue. It seems I'm only seeing this in a popover, so I'm not sure what's different.

@genepaul
Copy link
Contributor Author

genepaul commented Dec 8, 2021

Actually, I took the time to go through the versions of react-virtuoso in my codesandbox, and it seems the issue starts showing up with v1.10.7. The version before that, v.1.10.6, does not seem to have scrolling or gap issues.

@petyosi
Copy link
Owner

petyosi commented Dec 9, 2021

v1.10.7 switched from using offsetHeight to using getBoundingClientRect for item sizing. I'm not sure about the internals of the Expand component, but from what I see, the viewport size is not recalculated properly. This can happen for variety of reasons, including margins or animations.

It's unlikely for me to investigate that in a reasonable timeframe, especially given that it needs mui for reproduction (which in general has a lot going on in terms of styles and positioning). If you're looking for a workaround, you can try overriding itemSize to use offsetHeight (https://virtuoso.dev/virtuoso-api-reference/).

@genepaul
Copy link
Contributor Author

genepaul commented Dec 9, 2021

So yeah, it's definitely not getting the right height on mount. Here's what this looks like in my own code:

image

image

Above is what the elements look like in the DOM, but here's what Virtuoso thinks about the first one:

image

Interestingly, if I click any of the checkboxes, causing a re-render of the Virtuoso, it updates the data-known-size on all the list items to the correct value. So yeah, there's definitely an incorrect height getting reported. And indeed, when I log the height (either getBoundingClientRect or offsetHeight) in itemSize, it's incorrect when it initially mounts. I appreciate you looking at this, and it's definitely something with the interaction between Material and Virtuoso, so I don't expect you to dig into Material's code. But if any of this triggers any ideas for you, let me know. I'll keep digging. Thanks!

@genepaul genepaul closed this as completed Dec 9, 2021
@genepaul
Copy link
Contributor Author

genepaul commented Dec 9, 2021

Actually, one thing above is incorrect: in itemSize, el.offsetHeight is correct, but if I return that from itemSize I still have the rendering issue. So I'm not sure what else might be going on here.

@petyosi
Copy link
Owner

petyosi commented Dec 9, 2021

I'm not sure if the green part from the above is a margin - if so, it will confuse the layout a lot. offsetHeight does not include it.

@genepaul
Copy link
Contributor Author

genepaul commented Dec 9, 2021

I'm fairly certain it's padding. I tried to clear margins.

@genepaul
Copy link
Contributor Author

genepaul commented Jan 7, 2022

@petyosi, I determined what the issue is. It appears to be due to the fact that the Material UI Popover animates a transition when it enters. The Virtuoso tries to grab dimensions while the Popover is transitioning and gets incorrect information. If I defer the rendering of the Virtuoso until the Popover has fully entered, the Virtuoso works correctly. So this is not an issue with Virtuoso. I just wanted to add the update here for anyone else who happens upon this issue and wonders what the problem was. If I come up with working code I'll try to update with a codesandbox. I did a really dirty hack in my testing where I just did a setTimeout in a useEffect and that proved the issue. I think there's probably a better way to do it though. Thanks for creating such an awesome library!

@trainoasis
Copy link

@petyosi, I determined what the issue is. It appears to be due to the fact that the Material UI Popover animates a transition when it enters. The Virtuoso tries to grab dimensions while the Popover is transitioning and gets incorrect information. If I defer the rendering of the Virtuoso until the Popover has fully entered, the Virtuoso works correctly. So this is not an issue with Virtuoso. I just wanted to add the update here for anyone else who happens upon this issue and wonders what the problem was. If I come up with working code I'll try to update with a codesandbox. I did a really dirty hack in my testing where I just did a setTimeout in a useEffect and that proved the issue. I think there's probably a better way to do it though. Thanks for creating such an awesome library!

Did you manage to do it in a nicer way? I have the exact same issue :)

@genepaul
Copy link
Contributor Author

@trainoasis - I think I ended up moving away from Popover to using Popper and didn't have the same issue. I may have also moved away from flex display. I don't remember exactly, unfortunately. I've also since upgraded MUI and a lot of other libraries.

@Sushant-Rana
Copy link

@trainoasis Hi , facing a similar issue , any luck with the solution ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants