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

feat: support variable node height #238

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

Conversation

hasanayan
Copy link

@hasanayan hasanayan commented Mar 21, 2024

Hello, this is a first attempt at supporting variable row heights. I tried to test it with the existing examples and it seem to work fine with both exact and dynamic row heights.

@jameskerr
Copy link
Member

Very interesting! I am in the middle or re-working the code for a new version, so I am not going to merge this yet. But can you help me understand the steps you took?

Can you summarize the changes you've made in this branch?

What is the use case you needed the variable heights for?

@hasanayan
Copy link
Author

hasanayan commented Mar 28, 2024

Sure. We need to style our tree view so that rows have different height based on their levels or their order in the branch, that's why we needed this and we are currently using it with slight modifications from this PR. I was going to update this PR with our final changes and open others based on our needs but apparently now is not the right time. Do you have any date in mind to release the new version? I can tell you about our use cases.

Here is the summary of suggested changes in this PR;

  • Tree now accepts rowHeight as a number or a function that returns the height based on RowHeightCalculatorParams<T>
export type RowHeightCalculatorParams<T> = Pick<
  NodeApi<T>,
  | "childIndex"
  | "children"
  | "data"
  | "parent"
  | "id"
  | "rowIndex"
  | "tree"
  | "isRoot"
  | "isLeaf"
  | "isClosed"
  | "isOpen"
  | "isAncestorOf"
  | "level"
>;

the omitted properties of the NodeApi could be passed, too. I wasn't well versed with the library and I did not include those states so that I don't break anything I don't know. Plus, whatever you provide to rowHeight calculation function, you have to track places that might trigger changes to them it internally and re-draw the list after the change.(explained in the last point) and I didn't want to do that until there is a green light from you 😄

-DnD cursor positioning relies on computing top offset of specific rows which was a simple rowHeight*index before, now there is a method that iterates the list from the first element until 1 before the target element, computes their height and sums them as top offset

-It renders VariableSizeList instead of FixedSizeList and the only different prop is itemSize. Now, it should be a function that returns row height based on row index.

-Whenever a change is made on the list data (re-ordering, renaming, dnd etc), it calls VariableSizeList.resetAfterIndex() so that the heights of the row are updated

@jameskerr
Copy link
Member

Thank you for the detailed explanation. This is a good plan. I don't have a date for the new release. But I am actively working on it.

@josh-complex
Copy link

Would really love to see this feature 👍 We use dynamic row heights in our current unvirtualized tree, and working this into the current arborist implementation is rough to say the least

@simonpeters
Copy link

Would really love to see this feature 👍 We use dynamic row heights in our current unvirtualized tree, and working this into the current arborist implementation is rough to say the least

What library are you using?

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.

4 participants