fix(tabs): reposition tab body on direction change #12229
Conversation
devversion
added
i18n
googlebot
added
the
cla: yes
* Currently, if someone switches to RTL after the tabs have been rendered, the hidden tabs will not update their position. Meaning that the tabs will animate from the wrong side if a new tab is selected.
devversion
force-pushed
the
fix/tabs-reposition-tabs-body-dir-change
branch
from
1e1285b to
fa3eae6
Compare
src/lib/tabs/tab-body.ts
Outdated
| this._origin = 'right'; | ||
| constructor(private _elementRef: ElementRef, | ||
| @Optional() private _dir: Directionality, | ||
| // TODO(paul): make the changeDetectorRef required when doing breaking changes. |
There was a problem hiding this comment.
Mark this with a @deletion-target, otherwise we'll miss it when doing breaking changes.
There was a problem hiding this comment.
I wasn't sure about that because deletion-target kind of implies that we are going to delete that parameter.
src/lib/tabs/tab-body.ts
Outdated
|
|
||
| if ((dir == 'ltr' && this.origin <= 0) || (dir == 'rtl' && this.origin > 0)) { | ||
| return 'left-origin-center'; | ||
| } else { |
There was a problem hiding this comment.
The else here is redundant, you can just return at the bottom.
There was a problem hiding this comment.
Just took that logic from above, but makes sense. Will improve it.
| @@ -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()) { | |||
There was a problem hiding this comment.
From what I can tell you're never passing a dir to the calls of this method. Consider moving it into a variable.
There was a problem hiding this comment.
A dir is being passed from the subscribe in the constructor.
src/lib/tabs/tab-body.ts
Outdated
| private _positionIndex: number; | ||
|
|
||
| /** Subscription to the directionality change observable. */ | ||
| private _dirChangeSubscription: Subscription = Subscription.EMPTY; |
There was a problem hiding this comment.
The Subscription type here is inferred already.
devversion
force-pushed
the
fix/tabs-reposition-tabs-body-dir-change
branch
from
646cb31 to
99635e5
Compare
src/lib/tabs/tab-body.ts
Outdated
| changeDetectorRef?: ChangeDetectorRef) { | ||
|
|
||
| if (this._dir && changeDetectorRef) { | ||
| this._dir.change.subscribe(dir => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch. Missed that. Since dir is not always present, we need to either truthy check for the subscription or just use Subscription.EMPTY.
devversion
force-pushed
the
fix/tabs-reposition-tabs-body-dir-change
branch
from
99635e5 to
8a7a9c7
Compare
crisbeto
added
pr: lgtm
action: merge
* Due to angular#12229, the selected tab of a tab-group will incorrectly animate after initialization. This happens because the `MatTab` component by default assigns the `origin` to `null`. Right now the check does only handle `undefined` properly. Fixes angular#12455
* Due to angular#12229, the selected tab of a tab-group will incorrectly animate after initialization. This happens because the `MatTab` component by default assigns the `origin` to `null`. Right now the check does only handle `undefined` properly. Fixes angular#12455
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Note: I noticed that the
Tab group with dynamically changing tabsexample is not working properly anymore (this already happened before my changes; see docs). Will look into it.