tajo / react-movable

🔀 Drag and drop for your React lists and tables. Accessible. Tiny.
https://react-movable.pages.dev
MIT License
1.53k stars 51 forks source link

Issue with using index as key #60

Closed ErnoW closed 3 years ago

ErnoW commented 3 years ago

Currently, the index is used as key in the renderList. This can cause issues in the rendering though. https://github.com/tajo/react-movable/blob/4122ce73df606f81dcd1716d63989cc8fe2fdf51/src/List.tsx#L575

For example, when my 3rd component has an internal state. Lets say for example it can be expanded or collapsed. Then when I move this 3rd component to index 5. Then the 'new' 3rd component has the collapsed state of the old 3rd component.

This is because the 3rd component has the key 2 in both cases. So React thinks it is the same component. To fix this we need to provide a unique key to the component, that stays the same when moving the component (an id for example).

See: https://reactjs.org/docs/lists-and-keys.html

There are a few ways to solve this. I think that the most flexible way is to provide a getKey function to the List component. When this function is provided, we can assign the key by using: key: getKey(value). In this way it can support values that are strings/numbers/objects.

Let me know what you think, or if you think of other ways to solve this.

tajo commented 3 years ago

Since the value can be anything, it's pretty easy to use it for unique IDs:

  const [items, setItems] = useState([
    { label: "Item 1", key: "a" },
    { label: "Item 2", key: "b" },
    { label: "Item 3", key: "c" }
  ]);
  return (
    <List
      values={items}
      onChange={({ oldIndex, newIndex }) =>
        setItems(arrayMove(items, oldIndex, newIndex))
      }
      renderList={({ children, props }) => <ul {...props}>{children}</ul>}
      renderItem={({ value, props }) => (
        <li {...props} key={value.key}>
          {value.label}
        </li>
      )}
    />
  );
ErnoW commented 3 years ago

@tajo Thanks for your reply. Using a key from a property in the value object is exactly what I want indeed. But it seems that this is not working. Or I might be missing something.

It seems that internally, renderItem is called with these props:

const props: IItemProps = {
  key: index,
  tabIndex: isDisabled ? -1 : 0,
  'aria-roledescription': this.props.voiceover.item(index + 1),
  onKeyDown: this.onKeyDown,
  style: {
    ...baseStyle,
    visibility: isHidden ? 'hidden' : undefined,
    zIndex: isSelected ? 5000 : 0
  } as React.CSSProperties
};

Note that key is set to index. So setting the key does not work (at least not when using a custom component) See for example in this codesandbox (based on the custom component example): https://codesandbox.io/s/basic-react-movable-forked-jlj2r (Toggle one of the items and drag it to another position, the item is correctly moved but the internal is state not)

tajo commented 3 years ago

You have to flip key={value.key} and {...props} so the correct key gets applied (it goes from left to right).

https://codesandbox.io/s/basic-react-movable-forked-7mtsj

ErnoW commented 3 years ago

Perfect, thanks.