Skip to content

feat(tabs): provide the ability to render a tabpanel outside the tabset#1576

Open
spliffone wants to merge 1 commit intomainfrom
feat/external-tabpanel
Open

feat(tabs): provide the ability to render a tabpanel outside the tabset#1576
spliffone wants to merge 1 commit intomainfrom
feat/external-tabpanel

Conversation

@spliffone
Copy link
Member

@spliffone spliffone commented Feb 26, 2026

Describe in detail what your merge request does and why. Add relevant
screenshots and reference related issues via Closes #XY or Related to #XY.


@spliffone spliffone requested review from a team as code owners February 26, 2026 14:08
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the SiTabContentComponent to allow rendering tab content outside the SiTabsetComponent. It also updates the SiTabsetComponent to handle external content and adds corresponding tests. The changes enhance the flexibility of the tabs component. Feedback includes suggestions for improving the efficiency of signal computations, ensuring robustness in ARIA attribute handling, and adding a default case to a switch statement for better error handling.

Comment on lines +99 to +105
effect(() => {
const contentRef = this.content();
const active = this.activeTab();
if (contentRef) {
contentRef.panelId.set(active ? `content-${active.tabId}` : undefined);
contentRef.labelledBy.set(active ? `tab-${active.tabId}` : undefined);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The effect that pushes ARIA attributes to the external content component relies on active.tabId. If active is undefined, accessing active.tabId will result in an error. Add a check to ensure active is defined before accessing its properties.

Also, consider using takeUntilDestroyed to automatically unsubscribe from the effect when the component is destroyed to prevent memory leaks.

    effect(() => {
      const contentRef = this.content();
      const active = this.activeTab();
      if (contentRef && active) { // Add check for active
        contentRef.panelId.set( `content-${active.tabId}` );
        contentRef.labelledBy.set(`tab-${active.tabId}`);
      } else {
        contentRef?.panelId.set(undefined);
        contentRef?.labelledBy.set(undefined);
      }
    });

Comment on lines +77 to +79
readonly activeIndex = computed(() => {
const active = this.activeTab();
return active ? this.tabPanels().indexOf(active) : -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The activeIndex computed signal calculates the index of the active tab. If no tab is active, it returns -1. This is good. However, if the tabPanels signal changes (e.g., a tab is added or removed), this computation might trigger unnecessarily, even if the active tab hasn't changed. Consider memoizing the tabPanels signal or using a more efficient way to track the active index to avoid redundant computations.

Consider using computed(() => this.tabPanels().findIndex(tab => tab.active())) for efficiency.

Comment on lines +102 to +108
@switch (tabset.activeIndex()) {
@case (0) {
{{ selectedEntity | json }}
}
@case (1) {
<div class="mx-auto text-center">History for {{ selectedEntity.name }}</div>
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Consider adding a default case to the switch statement to handle unexpected activeIndex values. This can help prevent unexpected behavior if the activeIndex signal ever holds a value outside the expected range.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant