Expandable table requires parent/child references by row index, cannot be used with dynamic content

[AjayJagan]

The existing table design uses an array of objects([{},{},{}]) where each object would be a row.While trying to add a child, we need to place the child as the next element to the parent and the child should have a parent(as key) with the value as the index of the parent. If the child data is received asynchronously then using index would sometimes break the logic. Also the parent and the child are in the same level in the array. The suggested solution is to add a child key to the main parent object, thus parent-child encapsulation is present and there is no need to provide the parent index. Can any one from the team please help me with this issue

[srambach]

@mturley thanks for your help so far on this issue. Even if the parent-child key is changed not to use the row index, having parent and child at the same level and needing to have the child immediately after the parent requires recreation of the table and is a blocker for us to use the component. Having highly dynamic content in a table seems like a common use case. If others have a solution we'd welcome advice.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[mturley]

@tlabaj I think this might be a good candidate for a breaking change. Currently in Table, to create a collapsible row you need the hidden content to be added as a separate row with the parent row's index as a parent property. In cases like this where the rows are generated dynamically and can change, it would make more sense to be able to nest the child row definition in the parent row object. Maybe we could support both, though, and have it be non-breaking, but I'm not sure what that implementation would look like. cc @seanforyou23

[cristianonicolai]

This is something that Kogito needs for two applications, Management Console and Task Console, where we deal with parent and an unknown number of child items. Please let us know if we can provide any more details to help improve this component. @pefernan

[seanforyou23]

Chiming in - yes this can be a somewhat involved issue depending on how we want to think about it. I'm familiar with what we're discussing about the relationship of the indexes and parent/child data, but it's important to understand that "this is the way Table was architected" so to change it could likely involve a breaking change. Compound table, for example, also makes use of this same relationship between the data and the indexes. In case there's a simple solution that we just haven't found yet, can we ask that to provide a sample app that illustrates the problem in a minimal fashion? I get what's at stake from the description of the problem, but still, it would be really helpful to have an illustration of the problem. This would give us something concrete to test and write changes against.

having parent and child at the same level and needing to have the child immediately after the parent requires recreation of the table..

"requires recreation of the table"... I assume in other words, this means you have to re-render the while Table when the async data comes in, right? If I understand correctly, this is working as designed, but the way it has been designed maybe isn't ideal. Table was always data-down-events-up kind of thing, but yes, I've heard of many people hitting issues with this performance-wise.

[mturley]

@seanforyou23 I have an idea about how this could be implemented as a non-breaking change, but I'm not sure if the added complexity is worth it. In short, I think the problem with our table examples is that they all store the entire rows array in state, and mutate it in the format expected by the Table's rows prop directly. So if there is something we can't easily represent in that format (like true parent/child relationships) there is no good way to keep that state updated properly.

I think we should instead encourage people to store the data the rows are based on in props/state, in whatever format that makes sense to them, and transform that into the rows array as part of render logic. So a parent/child data structure could be stored as nested objects, and then at render time a wrapper component around the table could walk that structure to generate the rows array with indexes and pass that on to Table. Also, if most of the rows data comes from props and we are just duplicating it all into state so we can do something like mutate a selected boolean, we should instead have some state that maps row ids to selected booleans and just generate the rows from props and that. Too many of our examples show the antipattern of duplicating prop data into state.

My question is, is that just something we want to make a demo for, or is it complex and common enough that we would want to add some kind of logic to Table which will allow that parent/child structure to be passed in directly, optionally? I think it would be possible to make that backwards compatible if we have an optional prop in each row object like children and if present, transform the rows at the top level before passing them down as a flat array like before. The inner guts of Table wouldn't need to change. We could be more explicit with some kind of hasNestedRows flag maybe.

I've been having a hard time describing this, so I'm going to try and demonstrate what I mean in a CodeSandbox if I can. @AjayJagan @cristianonicolai can you share sandboxes of problematic code demonstrating a need for this?

[mturley]

@AjayJagan shared a CodeSandbox with me that demonstrates the issue: https://codesandbox.io/s/jolly-vaughan-0jq07?file=/src/table/table.js . You can see the problem if you open the two expandable rows very quickly.

To solve this problem, I rewrote this example using a more abstract solution. Like I describe above, instead of storing the entire rows array in state and trying to mutate it there (inject child rows into it and manage parent/child indexes), I use the underlying data for the parent and child items along with a small state object to track which rows are open, and generate a new rows array on the fly on each render. When each row is expanded, a child row with a spinner is shown, the child data is loaded in the background, and it is updated. It's all based on item ids rather than row indexes, so it can happen on multiple rows at the same time: https://codesandbox.io/s/pf-dynamic-expandable-table-bvuow?file=/src/table/CollapsibleTable.js

I think we should probably add this as a demo or an example in the docs. I'm starting to think maybe we don't need to change the Table component at all; it would be nice to be able to pass in structured data here instead of appending an extra row for the child, but the abstraction isn't complicated. We mostly just need to stop demonstrating storing and mutating the entire rows array in state.

[yzhao583]

@mturley Would you mind to share the codesandbox link again for the proposed solution? Looks like the original link does not work very well, it keeps loading forever. Thanks

[mturley]

@yzhao583 it seems to work fine for me. Maybe CodeSandbox was having trouble?

[AjayJagan]

@mturley - So if I understood it right, we are rendering a new table every time right?. So do you think it will affect the performance(for eg: when loading around 100 rows at a time and then try to toggle). Can you please share your thoughts on this.

[mturley]

@AjayJagan I do think we'll need to try it and see how the performance is, but re-rendering the entire table will not necessarily have as much of an impact as it would seem, since React's virtual DOM diffing should help. Having hundreds of rows in the document at the same time in the first place will probably have more impact than updating them, as long as the render logic for each row is not very expensive. But there are ways to cache/memoize those expensive parts if necessary. I'm really curious to see what we find when we try it.

Either way, it is probably a good idea to virtualize this table and avoid actually rendering that many rows at one time (as the user scrolls, load and unload them). Maybe we should explore options for that, like our community extension here: http://patternfly-react.surge.sh/documentation/react/extensions/virtualizedtable

I'm not certain this is the perfect approach, but I think a better solution would require a lot of modifications to the Table component, or a custom table implementation based on PF CSS (a lot of rework).

[AjayJagan]

@mturley - so just for starters, I tried to add around 100 process instances and tried to load the children. But the problem I encountered is that, there was a small delay when opening and closing the expandable table. In a real-world scenario, we will definitely be seeing more than 100 process instances and we could have some performance issues. Of course, we can use the Virtualized table but I just wanted to show you :). I have attached 2 videos for that. Please have a look.

[mturley]

Yep, that makes sense. It might be worth trying to use the debugger to figure out what the expensive parts of that render call are, and try to optimize where we can. It'll probably be unnecessary if the virtualized extension works for us though.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[mturley]

Shoo, stalebot.

[jschuler]

Came across this issue, I've recently added composable Table components and they appear more performant on the surface. I didn't dig in too much but I've created examples here: Heavyweight table: https://codesandbox.io/s/heavy-table-0i1vq

Composable table: https://codesandbox.io/s/composable-table-25wgu?file=/index.js

While not lightning fast, the composable table does expand the rows quicker.

Edit: Exploring more options (i.e. memoization) it's possible to make it even faster https://codesandbox.io/s/composable-table-forked-k5wyx?file=/index.js

[AjayJagan]

Came across this issue, I've recently added composable Table components and they appear more performant on the surface. I didn't dig in too much but I've created examples here: Heavyweight table: https://codesandbox.io/s/heavy-table-0i1vq

Composable table: https://codesandbox.io/s/composable-table-25wgu?file=/index.js

While not lightning fast, the composable table does expand the rows quicker.

Edit: Exploring more options (i.e. memoization) it's possible to make it even faster https://codesandbox.io/s/composable-table-forked-k5wyx?file=/index.js

@jschuler thanks a lot for this update. This helps me a lot in building the children without repainting the entire table. One question I would like to ask is if we could use react-virtualized with this table. If it is possible could you please provide me with an example.

[jschuler]

@AjayJagan yes, i have created examples http://patternfly-react.surge.sh/extensions/virtual-scroll-table#using-composable-table-components and http://patternfly-react.surge.sh/extensions/virtual-scroll-window-scroller#using-composable-table-components

[AjayJagan]

@AjayJagan yes, i have created examples http://patternfly-react.surge.sh/extensions/virtual-scroll-table#using-composable-table-components and http://patternfly-react.surge.sh/extensions/virtual-scroll-window-scroller#using-composable-table-components

Thanks!!!!

[AjayJagan]

@jschuler , is it possible to provide an example for virtualized-expandable-composable table component :)

[tlabaj]

Due to inactivity on this issue and the new table implementation we will close this issue.