Should `height` actually be `max-height`?

[tleunen]

There's a way to set a custom height for the list. This property is used to set the height in css. Would it make sense to change it for max-height?

I could eventually do something like this. But the lib should probably do it?

height={Math.min(items.length * 50, 300)}

[clauderic]

Hey @tleunen,

I'm not sure I understand what you mean, would you mind clarifying your question?

Currently, the height prop is used to determine how many items should be rendered on screen, and how many items should be virtualized. If you need to support a dynamic height, you can easily do so by calculating the height of the window or the parent container, and listening to resize events.

I don't think this is something that needs be included out of the box, as the main concern is to keep the footprint of the library as light as possible and only bundle functionality everyone will be using.

[tleunen]

Yep sorry if it wasn't clear. :)

Let's say you have a dynamic list. So sometimes, you have a lot of items, and some other times, you have only a few items.

You know that visually, you want the list to be max 300px height. So you'll set the height to 300. But then, if you only render 2 items which are 50px each. You'll see a 200px empty area.

So that's why I believe it should actually be a max-height instead of height when the height is set in the css.

height:

max-height:

<div style="overflow: auto; will-change: transform; max-height: 300px; width: 100%;"</div>

[clauderic]

Ahh, I see what you mean now. Thanks for providing screenshots.

Unfortunately this would only work if the children were relatively positioned. For performance reasons, the list items are absolutely positioned, so the parent needs to have a height property, otherwise the max-height calculation would default to 0

I think your best bet for a use-case like this is probably to either dynamically update the height prop based on the number of items, or not use virtualization at all if you have less then say, 5 items, and use a different component to render the children. You could mimic the same API if it makes things easier, and use the height prop to set the max-height.

For example:

const Component = (items.length > 5) ? VirtualList : List;

<Component
    width='100%'
    height={600}
    itemCount={data.length}
    itemSize={50} // Also supports variable heights (array or function getter)
    renderItem={({index, style}) =>
      <div key={index} style={style}> // The style property contains the item's absolute position
        Letter: {data[index]}, Row: #{index}
      </div>
    }
  />

[tleunen]

Hmm.. I'm not sure to understand the relative vs absolute position argument. I'm not talking about the height of an item, but the height of the list. Each item keeps its height.

Currently, I'm fixing it by calculating the height based on the number of items. Something like:

<VirtualList
  width='100%'
  height={Math.min(items.length * itemHeight, containerHeight)}
  itemSize={itemHeight}
  itemCount={items.length}
/>

But, if the height here was a max-height: https://github.com/clauderic/react-tiny-virtual-list/blob/master/src/index.js#L222 it would resolve the issue where the item list is < defined height.

<div ref={this._getRef} {...props} onScroll={this.handleScroll} style={{...STYLE_WRAPPER, ...style, maxHeight: height, width}}>

(maxHeight instead of height)

[clauderic]

Ah, I see, I thought you meant the height of the wrapper.

Hmm. Currently the wrapper is set to min-height: 100% and the root container has a fixed height.

I'm not convinced what you describe is the default behaviour most people will want, but here's what could be done to enable it: setting the max-height instead of height on the root container as you described, but also pass a min-height prop that would be passed to the wrapper.

This prop could be optional and default to the value of height, but if passed a value of null, it would behave the way you envisioned.

So in your use-case, you could do:

<VirtualList
  width='100%'
  height={containerHeight}
  minHeight={null}
  itemSize={itemHeight}
  itemCount={items.length}
/>

[tleunen]

Are you sure a custom minHeight would be required? From my testing, it works fine using maxHeight only

[clauderic]

It works fine for your use-case, yes. However I believe this would break the current default behaviour for everyone else