React-window: data param on itemSize func for VariableSizeList

[mattjohnsonpint]

Thanks for adding the data param on the itemKey func in 3c9a558.

I use the itemSize function in my usage of VariableSizeList, where the size of the item depends on the data in some way. It would be useful if the itemSize function also had a data param.

I can send a PR if you like, just wanted to run it by you first. Thanks.

[bvaughn]

This is the sort of thing I was afraid of, cascading feature requests 😅 I
don't think that parameter is necessary. Can you share a use case why you
would need it?

On Fri, Dec 14, 2018, 6:21 PM Matt Johnson <[email protected] wrote:

Thanks for adding the data param on the itemKey func in 3c9a558
https://github.com/bvaughn/react-window/commit/3c9a558b28ae9143c0bb5d376ede07b790f87c0f
.

I use the itemSize function in my usage of VariableSizeList, where the
size of the item depends on the data in some way. It would be useful if the
itemSize function also had a data param.

I can send a PR if you like, just wanted to run it by you first. Thanks.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/bvaughn/react-window/issues/109, or mute the thread
https://github.com/notifications/unsubscribe-auth/AABznd2wP5NanqlXOM7cTDdOdo1QjGRJks5u5FyvgaJpZM4ZUhu6
.

[bvaughn]

My current thinking is that I do not plan to add this functionality because I don't believe it is necessary, so I'm going to close this issue. If you have a compelling use case though, please share it with me. I'd be happy to talk about it– and perhaps reopen the issue.

[mattjohnsonpint]

Here is a simplified example:

<VariableSizeList
  height={totalHeight}
  itemData={items}
  itemCount={items.length}
  itemSize={(index) => items[index].itemType === 'header' ? 32 : 64 }
  itemKey={(index, data) => data[index].key }
  >
  {ItemRenderer}
</VariableSizeList>

class ItemRenderer extends React.PureComponent {
  render() {
    const item = this.props.data[this.props.index]

    return (
      <div style={this.props.style}>
        {
          item.itemType === 'header'
          ? <h2>{item.headerText}</h2>
          : <div>{item.otherStuff}</div>
        }
      </div>
    )
  }
}

The list of items is long (say, hundreds long), and they are grouped into a few sections, each with a header for that section. The section header is smaller in height than the item itself, so the item size is variable.

The above works fine. Just I was able to add the data param for itemKey, and as you can see it would be equally useful on itemSize.

[bvaughn]

You already have items in scope when you render your list, because you
reference items.length. Why can't you just use the in scope value for
itemSize?

On Mon, Dec 17, 2018, 7:17 PM Matt Johnson <[email protected] wrote:

Here is a simplified example:

height={totalHeight}
itemData={items}
itemCount={items.length}
itemSize={(index) => items[index].itemType === 'header' ? 32 : 64 }
itemKey={(index, data) => data[index].key }
>
{ItemRenderer}

class ItemRenderer extends React.PureComponent {
render() {
const item = this.props.data[this.props.index]

return (
  <div style={this.props.style}>
    {
      item.itemType === 'header'
      ? <h2>{item.headerText}</h2>
      : <div>{item.otherStuff}</div>
    }
  </div>
)

}
}

The list of items is long (say, hundreds long), and they are grouped into
a few sections, each with a header for that section. The section header is
smaller in height than the item itself, so the item size is variable.

The above works fine. Just I was able to add the data param for itemKey,
and as you can see it would be equally useful on itemSize.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/bvaughn/react-window/issues/109#issuecomment-448082217,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABznb3BlpOoR41gojKx0bqGLs8CtIccks5u6F49gaJpZM4ZUhu6
.

[mattjohnsonpint]

That's what I'm doing already without the data param, right? Or maybe I misunderstood you.

[bvaughn]

Yes. So why is it useful or necessary for either itemSize or itemKey?

[mattjohnsonpint]

I guess I was just looking for consistency. It seems odd to have it on one and not the other.

Also, with the data parameter, I can move these functions outside the render function. Though I suppose if my data is fully defined in state or props then I can still do that. It's only when the data is derived within the render function that it becomes useful.

I'm still learning React best practices. Some examples show creating data in the render function, and others say explicitly to avoid that. This one seems to indicate that its more of a problem when using PureComponent, but I'm not sure.

Anyway, API consistency is the only real reason I can think of. If you don't want to add it, that's fine. Thanks for the dialog. 👍

[bvaughn]

My strong preference is to avoid increasing the surface area of APIs unless
(1) it's necessary and (2) it's useful for more than edge cases.

I was not a fan of the itemKey change but I eventually gave in. I have a
similar stance on itemSize unless I can be convinced that it's not than
just theoretically useful. 😅

On Tue, Dec 18, 2018, 12:44 PM Matt Johnson <[email protected] wrote:

I guess I was just looking for consistency. It seems odd to have it on one
and not the other.

Also, with the data parameter, I can move these functions outside the
render function. Though I suppose if my data is fully defined in state or
props then I can still do that. It's only when the data is derived within
the render function that it becomes useful.

I'm still learning React best practices. Some examples show creating data
in the render function, and others say explicitly to avoid that. This one
https://codeburst.io/when-to-use-component-or-purecomponent-a60cfad01a81
seems to indicate that its more of a problem when using PureComponent,
but I'm not sure.

Anyway, API consistency is the only real reason I can think of. If you
don't want to add it, that's fine. Thanks for the dialog. 👍

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/bvaughn/react-window/issues/109#issuecomment-448363787,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABznVrX5FkfQ0SHUENOodjSKiXlMxzhks5u6VPEgaJpZM4ZUhu6
.

[jancama2]

Here is a simplified example:

<VariableSizeList
  height={totalHeight}
  itemData={items}
  itemCount={items.length}
  itemSize={(index) => items[index].itemType === 'header' ? 32 : 64 }
  itemKey={(index, data) => data[index].key }
  >
  {ItemRenderer}
</VariableSizeList>

class ItemRenderer extends React.PureComponent {
  render() {
    const item = this.props.data[this.props.index]

    return (
      <div style={this.props.style}>
        {
          item.itemType === 'header'
          ? <h2>{item.headerText}</h2>
          : <div>{item.otherStuff}</div>
        }
      </div>
    )
  }
}

The list of items is long (say, hundreds long), and they are grouped into a few sections, each with a header for that section. The section header is smaller in height than the item itself, so the item size is variable.

The above works fine. Just I was able to add the data param for itemKey, and as you can see it would be equally useful on itemSize.

@mj1856 Is this solution working for you even if the items are changing over time (adding, removing)? I have exactly the same type of list and I have an issue with itemSize being called only once when an item is rendered for the first time.

[stilllife00]

@bvaughn being new to the library I was also surprised by the lack of data prop (or just the item/row prop) as parameter for this function. I read your opinion on the matter and I just have a question if you might have time to clarify what is the best approach.

My surprise comes from the fact that this forces the usage of a closure like so:

itemSize={ index => {
    const myItem = myItems[index];
    // ... determine the size based on myItem
}}

As mentioned by others this makes the function less "movable" because it relies on and external variable myItems, but perhaps this is not such a strong point to complexify the library?

Also, I am always worried (perhaps mistakenly, please advise) that a closure like so is redefined at each render. That causes a new function to get passed down as props and this I thought is considered a bad practice because it causes unnecessary re-renders of the child component.

Is the solution in this case to memoize the closure with a useCallback ?
Is this optimization not so necessary?
Having more parameters in the library will solve this, but will perhaps just move the issue on another place causing a perf loss anyway?

Thanks