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

Include node info in rowHeight callback #199

Closed
fritz-c opened this issue Nov 18, 2017 · 11 comments
Closed

Include node info in rowHeight callback #199

fritz-c opened this issue Nov 18, 2017 · 11 comments

Comments

@fritz-c
Copy link
Member

fritz-c commented Nov 18, 2017

No description provided.

@fritz-c fritz-c self-assigned this Nov 18, 2017
@fritz-c fritz-c removed their assignment Nov 25, 2017
@lifejuggler
Copy link
Collaborator

@fritz-c I will make a pull request

@lifejuggler
Copy link
Collaborator

lifejuggler commented Nov 29, 2017

@fritz-c actually can you give me more context to this issue? or maybe I can pick up another issue since I am not sure what this issue is trying to do or solve

@fritz-c
Copy link
Member Author

fritz-c commented Dec 3, 2017

Sorry for the delay. The issue here is that when people want to set the height per row with the rowHeight prop by giving it a callback, it only gives the index within the currently displayed rows as an argument.

A usage example: if I wanted to have rows that were twice as tall for nodes with a doubleHeight: true attribute in them, on every render call I would have to generate the flattened tree while hiding collapsed nodes, just to figure out which node that index was pointing at.

// API as it is now:
({ index }) => rowHeight

// API as I think it should be
({ index, treeIndex, node, path }) => rowHeight
// Nice-to-haves: lowerSiblingCounts, isSearchMatch, isSearchFocus
//   -> Include these if they are available in the context and
//      don't require extra calculation

// * Note that `treeIndex` and `index` should be the same value,
// I just want the APIs to be consistent in their naming.

@C3PablO
Copy link

C3PablO commented Dec 14, 2017

Is there any way to go around this? to have different heights based based on node data?

@fritz-c
Copy link
Member Author

fritz-c commented Dec 14, 2017

@C3PablO Not right now. Would you be interested in adding the functionality? I believe it's only a matter of plugging in some extra variables into the callback call.

@C3PablO
Copy link

C3PablO commented Dec 14, 2017

yes, I think that would be very useful. For what I've seen around the issues, there was a fix for this in 1.3.0 (a906f21). No sure where that went. Another solution I'm looking at is the possibility of wrapping the cell into a CellMeasurer for rows where the height is difficult to guess so the node height is set based on the content. Think of it as a list of comments.

@fritz-c
Copy link
Member Author

fritz-c commented Dec 14, 2017

That fix you refer to only applied to non-virtualized trees. For whatever reason I didn't notice the virtualized version didn't get any attention.

@wuweiweiwu
Copy link
Member

@fritz-c if its not done already i can do it :)

@fritz-c
Copy link
Member Author

fritz-c commented Jan 13, 2018

@wuweiweiwu yes please!

@wuweiweiwu
Copy link
Member

#231 pull request submitted!

@fritz-c
Copy link
Member Author

fritz-c commented Jan 14, 2018

Available now in v1.6.0. Thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants