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

PF4 Table: lazy load support #1450

Closed
dlabrecq opened this issue Feb 26, 2019 · 10 comments
Closed

PF4 Table: lazy load support #1450

dlabrecq opened this issue Feb 26, 2019 · 10 comments
Labels
PF4 React issues for the PF4 core effort wontfix

Comments

@dlabrecq
Copy link
Member

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

@dgutride dgutride added PF4 React issues for the PF4 core effort and removed PF4 React issues for the PF4 core effort labels Feb 28, 2019
@karelhala
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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!

@dgutride dgutride added this to the PF rc.1 milestone Mar 18, 2019
@rachael-phillips rachael-phillips added p2 and removed p1 labels Mar 18, 2019
@rachael-phillips rachael-phillips modified the milestones: PF rc.1, PF rc.2 Mar 18, 2019
@rachael-phillips rachael-phillips removed this from the 1.0.0-rc-2 milestone May 14, 2019
@stale
Copy link

stale bot commented Jul 13, 2019

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

@stale stale bot added the wontfix label Jul 13, 2019
@dlabrecq
Copy link
Member Author

This is still an issue

@stale stale bot removed the wontfix label Jul 13, 2019
@stale
Copy link

stale bot commented Sep 11, 2019

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

@stale stale bot added the wontfix label Sep 11, 2019
@KKoukiou
Copy link
Collaborator

KKoukiou commented Sep 25, 2019

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.

@stale stale bot removed the wontfix label Sep 25, 2019
@dlabrecq
Copy link
Member Author

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 stale bot added the wontfix label Nov 24, 2019
@patternfly patternfly deleted a comment from stale bot Nov 25, 2019
@stale stale bot removed the wontfix label Nov 25, 2019
@stale stale bot added the wontfix label Jan 24, 2020
@patternfly patternfly deleted a comment from stale bot Jan 24, 2020
@stale stale bot removed the wontfix label Jan 24, 2020
@stale
Copy link

stale bot commented Mar 24, 2020

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

@stale stale bot added the wontfix label Mar 24, 2020
@stale stale bot closed this as completed Apr 7, 2020
@KKoukiou
Copy link
Collaborator

KKoukiou commented Apr 8, 2020

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PF4 React issues for the PF4 core effort wontfix
Projects
None yet
Development

No branches or pull requests

5 participants