lukasbach / react-complex-tree

Unopinionated Accessible Tree Component with Multi-Select and Drag-And-Drop
https://rct.lukasbach.com
MIT License
961 stars 77 forks source link

made item height for drag position dynamic #361

Open AustinMKerr opened 5 months ago

AustinMKerr commented 5 months ago

Solves #338

added itemsHeightArray to replace ItemHeight and refactored the code using this new const to make the drag and drop functions use a dynamic height of each tree's exact items instead using the static height of the first item then multiplying it as they had with itemHeight. I didn't delete itemHeight in case any function uses it, but likely nothing uses it.

Bug Fixes and Improvements

@austinmkerr

lukasbach commented 5 months ago

Hi, thank you very much for your contribution.

The tests currently fail on this branch, one of the reasons being that the layoutUtils.ts file is mocked away, but that can be fixed with a (computeItemHeightArray as jest.Mock).mockReturnValue(Array(10).fill(100)); call at the top of TestUtil.tsx. But there are more test errors where I didn't have time to find the reason yet.

I'm a bit hesistant to replace the current one-item-height logic, because it introduces more runtime effort (now item heights need to be checked for each of the rendered items, which doesn't scale that well for larger trees), and the benefit only comes for people that actually have trees with varying item heights, so most people will just get a performance degredation without benefit. But I would be fine to make this new height logic an optional opt-in, i.e. by default the old logic is used where the height of the first item is used for everything, and an optional prop like "hasVaryingItemHeights={true}" enables the logic where it uses the exact height for each item.

I also noticed that computeItemHeightArray is called in getHoveringPosition. getHoveringPosition runs each time the drag handler runs, which can be many times each second during drag. This will likely cause performance issues even on not so large trees, if possible we should definitely only run this only once when dragging starts, and not during each drag event.