Skip to content

fix(tabs): reposition tab body on direction change #12229

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
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/lib/tabs/tab-body.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ import {async, ComponentFixture, TestBed} from '@angular/core/testing';
import {MatRippleModule} from '@angular/material/core';
import {NoopAnimationsModule} from '@angular/platform-browser/animations';
import {MatTabBody, MatTabBodyPortal} from './tab-body';
import {Subject} from 'rxjs';


describe('MatTabBody', () => {
let dir: Direction = 'ltr';
let dirChange: Subject<Direction> = new Subject<Direction>();

beforeEach(async(() => {
dir = 'ltr';
Expand All @@ -21,7 +23,7 @@ describe('MatTabBody', () => {
SimpleTabBodyApp,
],
providers: [
{provide: Directionality, useFactory: () => ({value: dir})}
{provide: Directionality, useFactory: () => ({value: dir, change: dirChange})}
]
});

Expand Down Expand Up @@ -146,6 +148,22 @@ describe('MatTabBody', () => {
expect(fixture.componentInstance.tabBody._position).toBe('left');
});
});

it('should update position if direction changed at runtime', () => {
const fixture = TestBed.createComponent(SimpleTabBodyApp);

fixture.componentInstance.position = 1;
fixture.detectChanges();

expect(fixture.componentInstance.tabBody._position).toBe('right');

dirChange.next('rtl');
dir = 'rtl';

fixture.detectChanges();

expect(fixture.componentInstance.tabBody._position).toBe('left');
});
});


Expand Down
83 changes: 58 additions & 25 deletions src/lib/tabs/tab-body.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import {
Component,
ChangeDetectorRef,
Input,
Inject,
Output,
Expand Down Expand Up @@ -113,7 +114,17 @@ export class MatTabBodyPortal extends CdkPortalOutlet implements OnInit, OnDestr
'class': 'mat-tab-body',
},
})
export class MatTabBody implements OnInit {
export class MatTabBody implements OnInit, OnDestroy {

/** Current position of the tab-body in the tab-group. Zero means that the tab is visible. */
private _positionIndex: number;

/** Subscription to the directionality change observable. */
private _dirChangeSubscription: Subscription = Subscription.EMPTY;
Copy link
Member

Choose a reason for hiding this comment

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

The Subscription type here is inferred already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense.


/** Tab body position state. Used by the animation trigger for the current state. */
_position: MatTabBodyPositionState;

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

Expand All @@ -132,46 +143,43 @@ export class MatTabBody implements OnInit {
/** The tab body content to display. */
@Input('content') _content: TemplatePortal;

/** Position that will be used when the tab is immediately becoming visible after creation. */
@Input() origin: number;

/** The shifted index position of the tab body, where zero represents the active center tab. */
@Input()
set position(position: number) {
if (position < 0) {
this._position = this._getLayoutDirection() == 'ltr' ? 'left' : 'right';
} else if (position > 0) {
this._position = this._getLayoutDirection() == 'ltr' ? 'right' : 'left';
} else {
this._position = 'center';
}
this._positionIndex = position;
this._computePositionAnimationState();
}
_position: MatTabBodyPositionState;

/** The origin position from which this tab should appear when it is centered into view. */
@Input()
set origin(origin: number) {
if (origin == null) { return; }

const dir = this._getLayoutDirection();
if ((dir == 'ltr' && origin <= 0) || (dir == 'rtl' && origin > 0)) {
this._origin = 'left';
} else {
this._origin = 'right';
constructor(private _elementRef: ElementRef,
@Optional() private _dir: Directionality,
// TODO(paul): make the changeDetectorRef required when doing breaking changes.
Copy link
Member

Choose a reason for hiding this comment

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

Mark this with a @deletion-target, otherwise we'll miss it when doing breaking changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure about that because deletion-target kind of implies that we are going to delete that parameter.

changeDetectorRef?: ChangeDetectorRef) {

if (this._dir && changeDetectorRef) {
this._dir.change.subscribe(dir => {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this assign to the _dirChangeSubscription? Also if you're doing this in the constructor, you don't have to initialize it to Subscription.EMPTY.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Missed that. Since dir is not always present, we need to either truthy check for the subscription or just use Subscription.EMPTY.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

this._computePositionAnimationState(dir);
changeDetectorRef.markForCheck();
});
}
}
_origin: MatTabBodyOriginState;

constructor(private _elementRef: ElementRef,
@Optional() private _dir: Directionality) { }

/**
* After initialized, check if the content is centered and has an origin. If so, set the
* special position states that transition the tab from the left or right before centering.
*/
ngOnInit() {
if (this._position == 'center' && this._origin) {
this._position = this._origin == 'left' ? 'left-origin-center' : 'right-origin-center';
if (this._position == 'center' && this.origin !== undefined) {
this._position = this._computePositionFromOrigin();
}
}

ngOnDestroy() {
this._dirChangeSubscription.unsubscribe();
}

_onTranslateTabStarted(e: AnimationEvent): void {
const isCentering = this._isCenterPosition(e.toState);
this._beforeCentering.emit(isCentering);
Expand Down Expand Up @@ -202,4 +210,29 @@ export class MatTabBody implements OnInit {
position == 'left-origin-center' ||
position == 'right-origin-center';
}

/** Computes the position state that will be used for the tab-body animation trigger. */
private _computePositionAnimationState(dir: Direction = this._getLayoutDirection()) {
Copy link
Member

Choose a reason for hiding this comment

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

From what I can tell you're never passing a dir to the calls of this method. Consider moving it into a variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

A dir is being passed from the subscribe in the constructor.

if (this._positionIndex < 0) {
this._position = dir == 'ltr' ? 'left' : 'right';
} else if (this._positionIndex > 0) {
this._position = dir == 'ltr' ? 'right' : 'left';
} else {
this._position = 'center';
}
}

/**
* Computes the position state based on the specified origin position. This is used if the
* tab is becoming visible immediately after creation.
*/
private _computePositionFromOrigin(): MatTabBodyPositionState {
const dir = this._getLayoutDirection();

if ((dir == 'ltr' && this.origin <= 0) || (dir == 'rtl' && this.origin > 0)) {
return 'left-origin-center';
} else {
Copy link
Member

Choose a reason for hiding this comment

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

The else here is redundant, you can just return at the bottom.

Copy link
Member Author

@devversion devversion Jul 16, 2018

Choose a reason for hiding this comment

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

Just took that logic from above, but makes sense. Will improve it.

return 'right-origin-center';
}
}
}
2 changes: 1 addition & 1 deletion src/lib/tabs/tab-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ export class MatTabGroup extends _MatTabGroupMixinBase implements AfterContentIn
* a new selected tab should transition in (from the left or right).
*/
ngAfterContentChecked() {
// Clamp the next selected index to the boundsof 0 and the tabs length.
// Clamp the next selected index to the bounds of 0 and the tabs length.
// Note the `|| 0`, which ensures that values like NaN can't get through
// and which would otherwise throw the component into an infinite loop
// (since Math.max(NaN, 0) === NaN).
Expand Down