Skip to content

feat: expose replaceHistory option via navigable tab component #887

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

Merged
merged 5 commits into from
May 29, 2021

Conversation

arjunlalb
Copy link
Contributor

Description

expose replaceHistory option via navigable tab component

@arjunlalb arjunlalb requested a review from a team as a code owner May 28, 2021 16:06
@codecov
Copy link

codecov bot commented May 28, 2021

Codecov Report

Merging #887 (3c51f24) into main (5ea1852) will increase coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #887      +/-   ##
==========================================
+ Coverage   85.35%   85.36%   +0.01%     
==========================================
  Files         801      801              
  Lines       16473    16472       -1     
  Branches     2067     2067              
==========================================
+ Hits        14061    14062       +1     
+ Misses       2380     2378       -2     
  Partials       32       32              
Impacted Files Coverage Δ
...rc/tabs/navigable/navigable-tab-group.component.ts 50.00% <0.00%> (+5.55%) ⬆️
...ents/src/tabs/navigable/navigable-tab.component.ts 56.25% <100.00%> (+2.91%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ea1852...3c51f24. Read the comment docs.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@@ -57,7 +57,7 @@ export class NavigableTabGroupComponent implements AfterContentInit {
});

public onTabClick(tab: NavigableTabComponent): void {
this.navigationService.navigateWithinApp([tab.path], this.activatedRoute);
this.navigationService.navigateWithinApp([tab.path], this.activatedRoute, [], tab.replaceHistory);
Copy link
Contributor

Choose a reason for hiding this comment

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

did i accidentally forget to remove it? This function shouldn't be used. We need to add this option to the buildNavigationParams

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, this is nested inside an ht-link and should be deleted here and in the template

Copy link
Contributor Author

@arjunlalb arjunlalb May 29, 2021

Choose a reason for hiding this comment

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

@anandtiwary fixed this. Used ht-link in the component instead of hyperlink.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@arjunlalb arjunlalb merged commit 292c5c9 into main May 29, 2021
@arjunlalb arjunlalb deleted the replace-history-in-navigable-tab branch May 29, 2021 15:45
@github-actions
Copy link

Unit Test Results

    4 files  ±0  256 suites  ±0   14m 20s ⏱️ +58s
923 tests ±0  923 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
929 runs  ±0  929 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 292c5c9. ± Comparison against base commit 5ea1852.

@@ -14,17 +14,15 @@ import { NavigableTabComponent } from './navigable-tab.component';
<nav mat-tab-nav-bar *htLetAsync="this.activeTab$ as activeTab" disableRipple>
<ng-container *ngFor="let tab of this.tabs">
<ng-container *ngIf="!tab.hidden">
<ht-link [paramsOrUrl]="buildNavigationParam | htMemoize: tab">
<div class="tab-button" *htIfFeature="tab.featureFlags | htFeature as featureState">
<a mat-tab-link (click)="this.onTabClick(tab)" class="tab-link" [active]="activeTab === tab">
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure it is still able to set active attribute for mat-tab link? This is why I had kept the ht-link outside the mat-tab-link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. its setting active tab correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Thank you!

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.

3 participants