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

made item height for drag position dynamic #361

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AustinMKerr
Copy link

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

…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.
@lukasbach
Copy link
Owner

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.

@lukasbach lukasbach self-requested a review April 17, 2024 18:01
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

Successfully merging this pull request may close these issues.

2 participants