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(tabs): add lazy loading for tabpanel content #17388

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Adrii77
Copy link

@Adrii77 Adrii77 commented Jan 14, 2025

Defect Fixes

#16806
#17351

This PR add lazy loading for the desired tabpanel's projected content.

Inactive tabs are hidden([hidden]="!active())".
initialized signal is used to render content the first time the tab is active and prevent re-initalization on reselection. Maybe a linkedSignal could be used, but I write the code the be compatible with NG 18.
It could be really cool if this PR could be merged and available in a patch version on 18, since apps that migrate to Prime 18 end up with complex components instantiated inside inactive tabs... (this is my case)

Copy link

vercel bot commented Jan 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Updated (UTC)
primeng ⬜️ Ignored (Inspect) Visit Preview Jan 14, 2025 7:41pm
primeng-v18 ⬜️ Ignored (Inspect) Visit Preview Jan 14, 2025 7:41pm

Copy link

vercel bot commented Jan 14, 2025

Someone is attempting to deploy a commit to the primetek Team on Vercel.

A member of the Team first needs to authorize it.

@Adrii77 Adrii77 marked this pull request as draft January 14, 2025 15:52
@Adrii77 Adrii77 marked this pull request as ready for review January 14, 2025 15:55
@Adrii77 Adrii77 force-pushed the feat/add-lazy-loading-for-tabpanel-content branch 2 times, most recently from 2f838fc to 9c852ff Compare January 14, 2025 16:51
Copy link

@ArnaudFelixParis ArnaudFelixParis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Adrii77 Adrii77 marked this pull request as draft January 14, 2025 19:22
@Adrii77 Adrii77 force-pushed the feat/add-lazy-loading-for-tabpanel-content branch from 9c852ff to 706c99c Compare January 14, 2025 19:30
@Adrii77 Adrii77 force-pushed the feat/add-lazy-loading-for-tabpanel-content branch from 706c99c to 96e1111 Compare January 14, 2025 19:40
@Adrii77 Adrii77 marked this pull request as ready for review January 14, 2025 19:41
@mertsincan
Copy link
Member

Hi @Adrii77, thanks a lot for your PR! I think we can add lazy property for this feature in two components;

  1. In p-tabs component to configure globally
  2. In p-tabpanel component to configure self-decision

WDYT?

@mertsincan mertsincan added Status: Pending Review Issue or pull request is being reviewed by Core Team Status: Discussion Issue or pull request needs to be discussed by Core Team labels Jan 15, 2025
@Adrii77
Copy link
Author

Adrii77 commented Jan 15, 2025

Hi @mertsincan ,

Thank you for your answer and for your time :)

My opinion :

  1. Provide an ng-template is a self decision : you want a tab with lazy loading ? just project your content inside an ng-template. You want another tab without lazy loading ? don't encapsulate it.
  2. Smaller and simpler API, as we don't need to add an extra input for this behavior.
  3. Easier migration : with the old TabView component, lazy loading was working with ng-template. After migration to the new Tabs component, devs has just to remove pTemplate="content" from ng-template to keeping lazy loading.
  4. Today, applications that migrate to Prime 18 find themselves with a feature regression and a drop in performance, since lazy loading no longer exists. In my case, I have a page with 5 tabs which two of them contains a complex stateful component (with http calls). Even if one of those tab isn't active, components are initialized and backend calls are triggered.

With this PR, lazy loading is preserved and prevents this regression (working with my test, hope you'll test too !). I think in any case it would be wise and pragmatic to have lazy loading this way to prevent this feature regression and why not refactor later if you want to find a way to do it via inputs.

What do you think ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Discussion Issue or pull request needs to be discussed by Core Team Status: Pending Review Issue or pull request is being reviewed by Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants