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

Send data to itemKey function #90

Closed
ronag opened this issue Nov 17, 2018 · 8 comments
Closed

Send data to itemKey function #90

ronag opened this issue Nov 17, 2018 · 8 comments

Comments

@ronag
Copy link

ronag commented Nov 17, 2018

Send data to itemKey function. The same way as for the itemRenderer.

@bvaughn
Copy link
Owner

bvaughn commented Nov 17, 2018

Why?

It's useful to pass data to the item renderer because it's usually a separate function/class component, without shared scope. e.g.

class ComponentThatRendersAListOfItems extends PureComponent {
  render() {
    // Pass items array to the item renderer component as itemData:
    return (
      <FixedSizeList
        itemData={this.props.itemsArray}
        {...otherListProps}
      >
        {ItemRenderer}
      </FixedSizeList>
    );
  }
}
 
// The item renderer is declared outside of the list-rendering component.
// So it has no way to directly access the items array.
class ItemRenderer extends PureComponent {
  render() {
    // Access the items array using the "data" prop:
    const item = this.props.data[this.props.index];
 
    return (
      <div style={this.props.style}>
        {item.name}
      </div>
    );
  }
}

But the itemKey prop can just be an instance method, with access to the same data as would be passed along to the itemData prop, e.g.

class ItemRenderer extends PureComponent {
  itemKey = index => {
    // Find the item at the specified index:
    const item = items[index];
 
    // Return a value that uniquely identifies this item.
    // Typically this will be a UID of some sort.
    return item.id;
  };

  render() {
    // Access the items array using the "data" prop:
    const item = this.props.data[this.props.index];
 
    return (
      <div style={this.props.style}>
        {item.name}
      </div>
    );
  }
}

@ronag
Copy link
Author

ronag commented Nov 17, 2018

To avoid having to create a closure for the itemKey function.

@bvaughn
Copy link
Owner

bvaughn commented Nov 17, 2018

To avoid having to create a closure for the itemKey function.

Why is this a goal to be avoided? Why can't the itemKey function just be bound to the instance of your class component like I showed in my comment above?

@ronag
Copy link
Author

ronag commented Nov 17, 2018

@bvaughn You are right. I could do it with a class component. Just prefer using function components. I can do without. Just would have been nice.

@bvaughn
Copy link
Owner

bvaughn commented Nov 17, 2018

I'm just pushing back a little against the idea that creating a closure like this is actually that expensive or something that should be actively avoided. I don't think it is, unless we're creating it a lot of times.

I'm also reluctant to add any non-essential features to the API of react-window because I don't want it to end up bloated and hard to maintain like react-virtualized did. (Not that this particular issue would cause that problem, just offering some insight.)

Maybe it would be helpful if I saw an example of your code?

@ronag
Copy link
Author

ronag commented Nov 17, 2018

Hm, yes I see your points. Anyway, here is what I have:

const Grid = observer(function Grid ({
  store,
  store: {
    ref,
    itemsPerRow,
    itemSize,
    totalRows,
    containerWidth,
    containerHeight
  }
}) {
  return (
    <FixedSizeGrid
      columnCount={itemsPerRow}
      rowCount={totalRows + 1}
      columnWidth={itemSize.width}
      rowHeight={itemSize.height}
      width={containerWidth}
      height={containerHeight}
      ref={ref}
      itemKey={({ columnIndex, rowIndex }) => {
		const item = store.getItem(rowIndex * store.itemsPerRow + columnIndex)
		if (!item) {
		  // TODO (fix): Workaround issue with duplicate keys when item doesn't exists.	
          return columnIndex * rowIndex
		}
		return item.id
      }}
      itemData={store}
      style={{
        overflowX: 'hidden'
      }}
    >
      {({ data: store, columnIndex, rowIndex, style }) => {
        const item = store.getItem(rowIndex * itemsPerRow + columnIndex)
        return item ? (
          <div style={style}>
            <Tile item={item} />
          </div>
        ) : null
      }}
    </FixedSizeGrid>
  )
})

This is what I head in mind:

function ItemRenderer ({ data: store, columnIndex, rowIndex, style }) {
  const item = store.getItem(rowIndex * store.itemsPerRow + columnIndex)
  return item ? (
    <div style={style}>
      <Tile item={item} />
    </div>
  ) : null
}

function ItemKey ({ data: store, columnIndex, rowIndex }) {
  const item = store.getItem(rowIndex * store.itemsPerRow + columnIndex)
  if (!item) {
    // TODO (fix): Workaround issue with duplicate keys when item doesn't exists.
    return columnIndex * rowIndex
  }
  return item.id
}

const Grid = observer(function Grid ({
  store,
  store: {
    ref,
    itemsPerRow,
    itemSize,
    totalRows,
    containerWidth,
    containerHeight
  }
}) {
  return (
    <FixedSizeGrid
      columnCount={itemsPerRow}
      rowCount={totalRows + 1}
      columnWidth={itemSize.width}
      rowHeight={itemSize.height}
      width={containerWidth}
      height={containerHeight}
      ref={ref}
      itemKey={ItemKey}
      itemData={store}
      style={{
        overflowX: 'hidden'
      }}
    >
      {ItemRenderer}
    </FixedSizeGrid>
  )
})

@bvaughn
Copy link
Owner

bvaughn commented Nov 17, 2018

Thanks for sharing!

Let me sit on this a bit. My initial reaction to adding API features for "convenience" is generally slightly negative (unless it's really inconvenient) but sometimes that reaction changes after I've given it a little thought. 😄

@bvaughn
Copy link
Owner

bvaughn commented Dec 3, 2018

I gave this a lot of thought, and although I'm still not thrilled about passing the value– I think the request is reasonable enough that I felt wrong about pushing back.

So this change has been released in 1.3.1– there's now a data param for itemKey. 😄

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

No branches or pull requests

2 participants