Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Fix issues with tabs that are activated via an ngIf #1887

Merged
merged 9 commits into from
Aug 15, 2018

Conversation

Blackbaud-TrevorBurch
Copy link
Member

@codecov-io
Copy link

codecov-io commented Aug 8, 2018

Codecov Report

Merging #1887 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1887   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         414     414           
  Lines        8642    8643    +1     
  Branches     1279    1279           
======================================
+ Hits         8642    8643    +1
Impacted Files Coverage Δ
src/modules/tabs/tabset.component.ts 100% <100%> (ø) ⬆️

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 8cd1d9b...844ba53. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Aug 9, 2018

Codecov Report

Merging #1887 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1887   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         414     414           
  Lines        8663    8664    +1     
  Branches     1282    1282           
======================================
+ Hits         8663    8664    +1
Impacted Files Coverage Δ
src/modules/tabs/tabset.component.ts 100% <100%> (ø) ⬆️

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 c528532...adab7f3. Read the comment docs.

@Blackbaud-AlexKingman Blackbaud-AlexKingman self-assigned this Aug 15, 2018
Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up those tests. Just one question for you.

@@ -95,8 +95,10 @@ export class SkyTabsetComponent
// initialize each tab's index. (in case tabs are instantiated out of order)
this.tabs.forEach(item => item.initializeTabIndex());
this.tabs.changes.subscribe((change: QueryList<SkyTabComponent>) => {
change.filter(tab => tab.tabIndex === undefined)
.forEach(item => item.initializeTabIndex());
this.tabsetService.tabs.subscribe(currentTabs => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a nested subscription here? Could we get what we need by just listening to the service?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we need the first subscription here to know what has changed in the DOM . We then need the second subscription to know what the service currently recognizes. We then filter the DOM tabs to find only the ones that the service doesn't know about and then we initialize those in order to have the service recognize them.

I'm happy you asked the question though as I wasn't having the inner subscription close after the first output so it would have potentially led to memory issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 sounds good. nice catch on the inner sub.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants