Skip to content

Conversation

@alionazherdetska
Copy link
Contributor

@alionazherdetska alionazherdetska commented Sep 30, 2025

πŸ“„ Description

Ticket: #5940

Breaking Changes

  • Renamed post-tab-header component to post-tab-item
  • Renamed property panel β†’ name in post-tab-item
  • Renamed property name β†’ for in post-tab-panel
  • Renamed property activePanel β†’ activeTab in post-tabs
  • Changed slot structure: tab items now use default slot, panels use panels slot

New Features

  • Added navigation variant for anchor-based navigation
  • Automatic variant detection based on presence of anchor links
  • Navigation mode renders semantic <nav> element with proper ARIA attributes

Testing

Documentation

  • Updated Storybook stories with variant controls
  • Added examples for both panels and navigation variants

Animation:

  • Cleaned up animations to prevent memory leaks by canceling running animations on component destruction.

Note:
The controls table still shows both CSS Shadow Parts for both variants because Storybook automatically extracts these from the component's JSDoc part annotations at build time, preventing any dynamic filtering based on the selected variant control.

Unlike slots, which are manually defined in the stories configuration and support conditional visibility using the if property, CSS Shadow Parts are auto-generated from the component source code.

πŸš€ Preview Link

Tabs component


πŸ“ Checklist

  • βœ… My code follows the style guidelines of this project
  • πŸ› οΈ I have performed a self-review of my own code
  • πŸ“„ I have made corresponding changes to the documentation
  • ⚠️ My changes generate no new warnings or errors
  • πŸ§ͺ I have added tests that prove my fix is effective or that my feature works
  • βœ”οΈ New and existing unit tests pass locally with my changes

@changeset-bot
Copy link

changeset-bot bot commented Sep 30, 2025

πŸ¦‹ Changeset detected

Latest commit: a49862c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@swisspost/design-system-components Major
@swisspost/design-system-components-react Major
@swisspost/design-system-components-angular Major
@swisspost/design-system-documentation Minor
@swisspost/design-system-components-angular-workspace Patch
@swisspost/design-system-nextjs-integration Patch
@swisspost/design-system-styles Major
@swisspost/design-system-tokens Major
@swisspost/design-system-icons Major
@swisspost/design-system-styles-primeng Major
@swisspost/internet-header Major
@swisspost/design-system-styles-primeng-workspace Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@swisspost-bot
Copy link
Contributor

swisspost-bot commented Sep 30, 2025

Related Previews

@alionazherdetska alionazherdetska marked this pull request as draft September 30, 2025 11:15
@alionazherdetska alionazherdetska marked this pull request as draft October 16, 2025 14:53
@alionazherdetska alionazherdetska marked this pull request as ready for review October 17, 2025 11:38
@alizedebray alizedebray requested review from leagrdv and removed request for alizedebray October 28, 2025 14:44
Copy link
Contributor

@leagrdv leagrdv left a comment

Choose a reason for hiding this comment

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

The previous Subnavigation component had styles. Don't we want the anchor navigation to also have styles? Since it's sharing the post-tabs component, I'd have expected it to have the same styles

role={isPanelMode ? 'tab' : undefined}
data-version={version}
data-navigation-mode={this.isNavigationMode.toString()}
aria-selected={isPanelMode ? 'false' : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

In anchor mode, the active link should have aria-current="page"


render() {
const tabsRole = this.isNavigationMode ? undefined : 'tablist';
const ariaLabel = this.isNavigationMode ? 'Tabs navigation' : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

aria-label should be localized so that should be added as a prop

Copilot AI review requested due to automatic review settings November 3, 2025 14:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the post-tabs component to support two distinct usage modes: panels variant (for tabbed content sections) and navigation variant (for anchor-based page navigation). The refactoring includes renaming post-tab-header to post-tab-item, updating property names for consistency, and implementing automatic mode detection based on the presence of anchor elements.

  • Renamed post-tab-header component to post-tab-item with updated properties
  • Changed property names across components for better clarity (activePanel β†’ activeTab, panel β†’ name, name β†’ for)
  • Added automatic mode detection and dual-mode support (panels vs navigation)

Reviewed Changes

Copilot reviewed 26 out of 28 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
packages/components/src/components/post-tabs/post-tabs.tsx Core refactoring with mode detection logic and navigation support
packages/components/src/components/post-tab-item/post-tab-item.tsx New component replacing post-tab-header with navigation mode detection
packages/components/src/components/post-tab-panel/post-tab-panel.tsx Updated property from name to for and added slot attribute
packages/documentation/src/stories/components/tabs/tabs.stories.ts Updated stories to demonstrate both variants with new API
packages/nextjs-integration/src/app/ssr/page.tsx Updated integration example with new component names and properties
packages/components/cypress/e2e/tabs.cy.ts Comprehensive test updates covering both modes and accessibility
Various documentation files Updated all documentation references from old to new API

}

// Clean up content observer
this.contentObserver.disconnect();
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

The contentObserver may be undefined if setupContentObserver() throws an error or if disconnectedCallback() is called before componentDidLoad() completes. Add a null check before calling disconnect().

Suggested change
this.contentObserver.disconnect();
if (this.contentObserver) {
this.contentObserver.disconnect();
}

Copilot uses AI. Check for mistakes.
Comment on lines 108 to +218
private enableTabs() {
// Prevent early call before detectMode()
if (!this.isLoaded) return;
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

The guard if (!this.isLoaded) causes enableTabs() to exit early when called from componentDidLoad() at line 65, before this.isLoaded = true is set at line 64. This means tabs won't be enabled on initial load. Move this.isLoaded = true before calling enableTabs() in componentDidLoad().

Copilot uses AI. Check for mistakes.
Comment on lines +178 to +181
if (this.isNavigationMode) {
if (this.isLoaded) this.postChange.emit(this.currentActiveTab.name);
return;
}
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

In navigation mode, if this.isLoaded is false, the event won't be emitted. However, the method still returns, which means navigation tabs activated during initialization won't trigger the postChange event. This is inconsistent with panels mode behavior at line 202. Consider removing the if (this.isLoaded) check or handling both modes consistently.

Copilot uses AI. Check for mistakes.
shadow: true,
})
export class PostTabItem {
private mutationObserver = new MutationObserver(this.checkNavigationMode.bind(this));
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

The MutationObserver callback binding is created during field initialization before the component instance is fully constructed. Consider initializing the observer in connectedCallback() to ensure proper binding context: this.mutationObserver = new MutationObserver(this.checkNavigationMode.bind(this));

Copilot uses AI. Check for mistakes.
name: 'active-panel',
variant: {
name: 'variant',
description: 'Select between panels variant (content sections) or navigation variant (page navigation). <post-banner data-size="sm"><p>If you attempt to mix both variants(anchors + panels), the component will throw an error.</p></post-banner>',
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

Missing space in 'variants(anchors' - should be 'variants (anchors'.

Suggested change
description: 'Select between panels variant (content sections) or navigation variant (page navigation). <post-banner data-size="sm"><p>If you attempt to mix both variants(anchors + panels), the component will throw an error.</p></post-banner>',
description: 'Select between panels variant (content sections) or navigation variant (page navigation). <post-banner data-size="sm"><p>If you attempt to mix both variants (anchors + panels), the component will throw an error.</p></post-banner>',

Copilot uses AI. Check for mistakes.
Comment on lines +276 to 278

activeItem?.remove();
activePanel?.remove();
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

The find() method returns undefined if no active item is found, but the type annotation declares it as HTMLPostTabItemElement | undefined. The subsequent check at line 272 guards against this, but the logic appears flawed - find() should use a proper predicate. The current implementation always returns the first item because the arrow function doesn't return the result of classList.contains('active').

Copilot uses AI. Check for mistakes.
});

it('should render the tabs container as nav element', () => {
cy.get('@tabs').find('nav[role="navigation"], nav').should('exist');
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

The selector nav[role=\"navigation\"], nav is redundant since <nav> elements have an implicit ARIA role of 'navigation'. The test should either check for just nav or verify that the role is properly set/omitted. Consider simplifying to cy.get('@tabs').find('nav').should('exist').

Suggested change
cy.get('@tabs').find('nav[role="navigation"], nav').should('exist');
cy.get('@tabs').find('nav').should('exist');

Copilot uses AI. Check for mistakes.
@@ -1,4 +1,4 @@
import * as Components from '@swisspost/design-system-components/dist';
import * as Components from '@swisspost/design-system-components/loader';
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

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

The import path changed from dist to loader. This appears to be an unrelated change to the tabs refactoring. Ensure this import path change is intentional and doesn't break the component loading mechanism.

Suggested change
import * as Components from '@swisspost/design-system-components/loader';
import * as Components from '@swisspost/design-system-components/dist';

Copilot uses AI. Check for mistakes.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 3, 2025

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.

5 participants