patternfly / patternfly-react

A set of React components for the PatternFly project.
https://react-staging.patternfly.org/
MIT License
794 stars 356 forks source link

PF4 Table: lazy load support #1450

Closed dlabrecq closed 4 years ago

dlabrecq commented 5 years ago

As a user, I want the expanding rows to be lazily loaded.

In the Cost Management UI, each expandable row child generates multiple requests (up to 3 each) required to populate charts, progress bars etc.

While implementing the PF4 table, we noticed that all expandable row children are rendered at the same time. This means all requests are generated immediately, when the page is loaded, regardless if rows are expanded or not. This costs us in performance, taking up to 20+ seconds for the page to load.

As a workaround, I've implemented code to return null instead of providing a child component. This forces the row to lazy load when expanded, which is what we want. Instead of generating all possible requests, we only see 3 for the current row. Requests are cached, so this happens only once, as needed.

Ideally, it would be great if the table component did this for us -- we could omit our custom code to handle this. For example, the table component might omit rendering children instead of just hiding them? We could even use a flag, like isLazyLoad, to enable this as a feature.

Below is an example of how we return null for lazy loading via our handleOnCollapse function.

screen shot 2019-02-26 at 9 48 05 am

karelhala commented 5 years ago

This might be good example of custom table extension. But definetely not part of core implementation, the lazy load could be done in various ways. Either by passing the node that consumer wants to render or by passing function that will be called whenever user clicks on expand. And while this row is being loaded show some custom loading mechanism, like some spinner or component etc.

Btw nice improvement of my suggestion from https://github.com/project-koku/koku-ui/issues/469.

dlabrecq commented 5 years ago

I filed this issue after a conversation with @dgutride about the default rendering behavior for table and tabs. Note that this would not change existing behavior, just add an optional flag to lazily load each expanding row.

Can we please have a discussion with the PatternFly team before shooting this down?

karelhala commented 5 years ago

It's really good idea, I just don't think that it should be part of core implementation but rather specific package with extension. Let's use reactabular as much as possible!

stale[bot] commented 5 years ago

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

dlabrecq commented 5 years ago

This is still an issue

stale[bot] commented 5 years ago

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

KKoukiou commented 5 years ago

This issue is actually quite important and I really believe it should not be an extension rather a part of the built in functionality.

The WA provided above is not ideal since whenever the row will collapse to closed and to open again the whole expandable part of the component needs to be mounted from scratch. That means that the expanded component will lose its state when collapsed to closed, which in many cases is not desirable. @dlabrecq please correct me if I am wrong.

In our case, (https://github.com/cockpit-project/cockpit) tables' length can grow quite a lot and the content of each expandable row is a component that is not really cheap to load, doing all the loads unconditionally when mounting the Table component can really deteriorate the performance.

We need to expose some attr on the IRow component will which define the behavior regarding the loading of the expandable components.

dlabrecq commented 5 years ago

Two new props; mountOnEnter and unmountOnExit, were just added to Tabs in support of this functionality -- hoping we can apply something similar for expandable rows?

stale[bot] commented 4 years ago

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

KKoukiou commented 4 years ago

I don't think this should be close - it would be a quite big performance gain to be able to control this on some specific use cases. @tlabaj can you please reopen?