chore(tabs): use custom portal template directive for hosting tab body content #7266

josephperrott · 2017-09-22T20:16:57Z

Related to #6832
By moving the entrance logic into the MdTabBodyPortal setting up the content after change detection is avoided.

Fixes #7032 #7061

amcdnl · 2017-09-22T21:05:28Z

src/lib/tabs/tab-body.ts


  /** Event emitted when the tab begins to animate towards the center as the active tab. */
  @Output() onCentering: EventEmitter<number> = new EventEmitter<number>();

+  /** Event emitted when the tab completes its animation away from the center. */
+  @Output() afterLeavingCenter: EventEmitter<number> = new EventEmitter<number>();


Can we prefix this w/ a underscore since its private ... so _afterLeavingCenter?

Whats the number type used for?

Done, and the number was the result of a copy paste from the emitter above. Since I wasn't using the result at all I didn't notice.

josephperrott · 2017-09-22T21:26:45Z

@andrewseguin If you have a moment to take a look. :D

andrewseguin · 2017-09-22T22:11:52Z

src/lib/tabs/tab-body.spec.ts

      fixture.componentInstance.position = 1;
      fixture.detectChanges();
+      tick(1000);


Leave comment explaining where 1000 came from and why tick was needed

andrewseguin · 2017-09-22T22:13:09Z

src/lib/tabs/tab-body.ts

+export class MdTabBodyPortal extends _MdTabBodyPortalBaseClass implements OnInit {
+  /** Whether the tab body should be visible. */
+  visible = false;
+  /** A subscription to events for when the tab body begins centering. */


Looks like the subs should be private (preceeded with an underscore)

andrewseguin · 2017-09-22T22:13:33Z

src/lib/tabs/tab-body.ts

+  constructor(
+    _componentFactoryResolver: ComponentFactoryResolver,
+    _viewContainerRef: ViewContainerRef,
+    private host: MdTabBody) {


Add underscore to host to show its private

andrewseguin · 2017-09-22T22:14:06Z

src/lib/tabs/tab-body.ts

+  /** Set up subscriptions for changing visibility and set initial visibility. */
+  ngOnInit(): void {
+    this.centeringSub = this.host.onCentering.subscribe(() => this.setVisibility(true));
+    this.leavingCenterSub = this.host._afterLeavingCenter.subscribe(


this.leavingCenterSub = this.host._afterLeavingCenter.subscribe(() => this.setVisibility(false));

andrewseguin · 2017-09-22T22:16:30Z

src/lib/tabs/tab-group.spec.ts

      expect(getSelectedLabel(fixture).textContent).toMatch('Junk food');
      expect(getSelectedContent(fixture).textContent).toMatch('Pizza, fries');

      tabGroup.selectedIndex = 2;
      fixture.detectChanges();
+      fixture.whenStable().then(() => {


Add comment why whenStable is needed

andrewseguin · 2017-09-22T22:17:57Z

src/lib/tabs/tabs.spec.ts

@@ -0,0 +1,44 @@
+import {Component} from '@angular/core';


Not sure why the extra test is needed, isn't this covered by tab-group.spec.ts?

It was not caught by the current tests in tab-group.spec.ts, would you prefer I move it into tab-group.spec.ts?

andrewseguin · 2017-09-22T22:18:32Z

src/lib/tabs/tab-body.ts

@@ -175,7 +220,7 @@ export class MdTabBody implements OnInit, AfterViewChecked {
  }

  /** Whether the provided position state is considered center, regardless of origin. */
-  private _isCenterPosition(position: MdTabBodyPositionState|string): boolean {
+  _isCenterPosition(position: MdTabBodyPositionState|string): boolean {


Will need to remove the underscore if it's not private.

andrewseguin

LGTM - nice work, takes a lot to get a good hold of all the change detection pieces happening in the tabs component

andrewseguin · 2017-09-22T23:00:31Z

Add merge_ready after you move the test to tab-group.spec.ts

mmalerba · 2017-09-25T01:39:33Z

src/lib/tabs/tab-body.ts

+  constructor(
+    _componentFactoryResolver: ComponentFactoryResolver,
+    _viewContainerRef: ViewContainerRef,
+    private _host: MdTabBody) {


@josephperrott it looks like we need to use a forwardRef here since this directive is used in the template for MdTabBody. Here's an example for a similar situation in the sidenav: https://github.com/angular/material2/blob/master/src/lib/sidenav/sidenav.ts#L36

mmalerba · 2017-09-26T17:35:11Z

@josephperrott This seems to break tab animations in some cases. Go to tabs demo page under "Tab Group Demo - Dynamic Tabs" and click "Tab 4" to see the broken behavior

josephperrott · 2017-09-26T20:42:55Z

@mmalerba Issue resolved.

amcdnl · 2017-10-03T14:50:36Z

@josephperrott - Can you rebase?

angular-automatic-lock-bot · 2019-09-07T14:02:02Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

josephperrott requested a review from andrewseguin as a code owner September 22, 2017 20:16

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 22, 2017

josephperrott force-pushed the tabs branch 2 times, most recently from 2ce1567 to f552539 Compare September 22, 2017 20:35

amcdnl reviewed Sep 22, 2017

View reviewed changes

josephperrott force-pushed the tabs branch from f552539 to a43358a Compare September 22, 2017 21:11

josephperrott added P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful pr: needs review labels Sep 22, 2017

josephperrott assigned josephperrott and andrewseguin and unassigned josephperrott Sep 22, 2017

andrewseguin reviewed Sep 22, 2017

View reviewed changes

josephperrott force-pushed the tabs branch from a43358a to a90c3df Compare September 22, 2017 22:55

andrewseguin approved these changes Sep 22, 2017

View reviewed changes

andrewseguin added pr: lgtm and removed pr: needs review labels Sep 22, 2017

josephperrott force-pushed the tabs branch from a90c3df to e8f72d1 Compare September 22, 2017 23:12

josephperrott added the action: merge The PR is ready for merge by the caretaker label Sep 22, 2017

josephperrott force-pushed the tabs branch from e8f72d1 to 1ed2d75 Compare September 22, 2017 23:13

mmalerba reviewed Sep 25, 2017

View reviewed changes

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Sep 25, 2017

josephperrott force-pushed the tabs branch 2 times, most recently from 4dfdf85 to d587250 Compare September 25, 2017 15:32

mmalerba added the action: merge The PR is ready for merge by the caretaker label Sep 25, 2017

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Sep 26, 2017

josephperrott force-pushed the tabs branch from d587250 to 1bf8eb9 Compare September 26, 2017 20:42

josephperrott added the action: merge The PR is ready for merge by the caretaker label Sep 28, 2017

josephperrott force-pushed the tabs branch from 1bf8eb9 to 4f46564 Compare September 29, 2017 00:18

amcdnl mentioned this pull request Oct 3, 2017

feat(tabs): adds ability for lazy loaded tabs #6832

Closed

josephperrott force-pushed the tabs branch from 4f46564 to 41f133c Compare October 3, 2017 15:39

kara added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Oct 3, 2017

josephperrott force-pushed the tabs branch 2 times, most recently from 2a8a5e8 to b894fe8 Compare October 4, 2017 18:06

jelbourn added P2 The issue is important to a large percentage of users, with a workaround and removed P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful labels Oct 24, 2017

Move tabs to smart portal directive.

1a07081

josephperrott force-pushed the tabs branch from b894fe8 to 1a07081 Compare November 2, 2017 18:25

josephperrott mentioned this pull request Nov 6, 2017

feat(expansion-panel): allow for content to be rendered lazily #8243

Merged

josephperrott merged commit 0e03bf4 into angular:master Nov 10, 2017

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

chore(tabs): use custom portal template directive for hosting tab body content #7266

chore(tabs): use custom portal template directive for hosting tab body content #7266

Uh oh!

Conversation

josephperrott commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephperrott commented Sep 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewseguin left a comment

Choose a reason for hiding this comment

Uh oh!

andrewseguin commented Sep 22, 2017

Uh oh!

mmalerba Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmalerba commented Sep 26, 2017

Uh oh!

josephperrott commented Sep 26, 2017

Uh oh!

amcdnl commented Oct 3, 2017

Uh oh!

angular-automatic-lock-bot bot commented Sep 7, 2019

Uh oh!

Uh oh!

josephperrott commented Sep 22, 2017 •

edited

Loading

mmalerba Sep 25, 2017 •

edited

Loading