fix(tabs): allow tab selection without content panel and remove router dependency #16169
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Lipata <[email protected]>
Co-authored-by: Lipata <[email protected]>
Copilot
AI
changed the title
Copilot finished work on behalf of
Lipata
August 21, 2025 13:08
|
@copilot can you check the 4 failing tests in Node.js CI and adapt them to the new tab behavior? |
Copilot stopped work on behalf of
Lipata due to an error
August 21, 2025 13:25
|
@copilot you can chech the package.json or yaml files, to see how tests are run: |
Co-authored-by: Lipata <[email protected]>
... I've fixed the 4 failing tests in Node.js CI and adapted them to the new tab behavior. The issue was that my original fix was too broad - it affected routing tabs when it should only affect action tabs without content. The fix now distinguishes between different types of tabs:
All test suites now pass:
Fixed in commit 9d52053. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
Copilot finished work on behalf of
Lipata
August 21, 2025 13:38
|
@copilot, even though the currently failing test is not part of the change, it seems like a flickering one. Do you have a suggestion to resolve it: IgxCustomNgElementStrategy
✗ should populate parent's content query
Expected [ ] to contain _a142({ grid: _a236({ validation: IgxGridValidationService2({ _validityStates: Map( ), _valid: true, grid: <circular reference: Object> }), selectionService: _a71({ zone: _NgZone({ hasPendingMacrotasks: true, hasPendingMicrotasks: false, isStable: true, onUnstable: EventEmitter_({ closed: false, currentObservers: [ SafeSubscriber({ initialTeardown: undefined, closed: false, _parentage: _Subscription({ initialTeardown: undefined, closed: false, _parentage: null, _finalizers: Array }), _finalizers: [ _Subscription ], isStopped: false, destination: ConsumerObserver({ partialObserver: Object }) }), SafeSubscriber({ initialTeardown: undefined, closed: false, _parentage: _Subscription({ initialTeardown: undefined, closed: false, _parentage: null, _finalizers: Array }), _finalizers: [ _Subscription ], isStopped: false, destination: ConsumerObserver({ partialObserver: Object }) }) ], observers: [ SafeSubscriber({ initialTeardown: undefined, closed: false, _parentage: _Subscription({ initi ....
at <Jasmine>
at projects/igniteui-angular-elements/src/app/custom-strategy.spec.ts:56:56
at Generator.next (<anonymous>)
at fulfilled (chunk-BKI2KXXQ.js:58:24)
at _ZoneDelegate.invoke (node_modules/zone.js/fesm2015/zone.js:398:28)
|
…m-strategy test Co-authored-by: Lipata <[email protected]>
... I've fixed the flickering test "IgxCustomNgElementStrategy should populate parent's content query" by replacing the fixed timer delays with a retry mechanism that waits for the actual component state. Root cause: The test was using fixed delays (20ms, 30ms) which weren't sufficient on slower systems or under load conditions. Solution:
This eliminates the race condition that caused the intermittent failures while maintaining the same test coverage. Fixed in commit f9c62ec. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot, please revert the last commit! |
Copilot
AI
changed the title
Copilot finished work on behalf of
Lipata
August 21, 2025 14:02
Co-authored-by: Lipata <[email protected]>
Copilot finished work on behalf of
Lipata
August 21, 2025 14:05
Lipata
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where tabs without content panels couldn't be selected by clicking their headers, while preserving existing routing behavior for tabs with RouterLink directives.
- Modifies click handler logic to distinguish between action tabs, routing tabs, and content tabs
- Adds comprehensive test coverage for tabs without content
- Includes a showcase example demonstrating the new functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tab-header.directive.ts | Enhanced click handler to detect RouterLink directive and allow selection for non-routing tabs |
| tabs.component.spec.ts | Added test case verifying tab selection works for tabs without content panels |
| tabs-showcase.sample.html | Added example showcasing tabs without content to demonstrate the fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@copilot I don't want to import router link, because this will force dependancy of it in the library. I prefer leaving the inital fix and also changing the existing tests - emits in test should consider that now events are fired, etc. |
…ting tabs Co-authored-by: Lipata <[email protected]>
Removed RouterLink dependency and reverted to the initial fix as requested. Updated the existing routing tests to expect the new behavior where events are fired immediately when clicking tabs. The library now avoids the Router dependency while preserving all functionality. Fixed in commit 9941929. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
Copilot finished work on behalf of
Lipata
August 21, 2025 19:23
|
@copilot still there are 2 failing tests that are expecting router guard, when there isn't a content. This means that test can be transformed to check tab selection change and rename the test, if such tests already exist, to remove those tests. |
…d behavior Co-authored-by: Lipata <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot finished work on behalf of
Lipata
August 21, 2025 20:06
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #16067
Problem
Previously, tabs without
igx-tab-contentcould not be selected by clicking on their headers due to a check forthis.tab.panelComponentin the click handler. This prevented valid use cases like:Solution
Removed the
if (this.tab.panelComponent)check in the tab header click handler to allow all tabs to be selected when clicked, regardless of whether they have content panels.The implementation avoids adding a dependency on Angular Router by keeping the fix simple and updating existing tests to accommodate the new behavior where routing tabs now fire selection events immediately when clicked.
Key Changes
igx-tab-content) - continue working as before ✅routerLink) - both navigate AND select immediately when clicked ✅Testing
This enables flexible tab implementations while maintaining backward compatibility and avoiding unnecessary dependencies.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.