feat(tabs): add animation when switching tabs, include optional dynamic height #1788

andrewseguin · 2016-11-09T18:40:16Z

No description provided.

hansl

Lots of little points, but overall very good 👍

hansl · 2016-11-09T18:42:21Z

src/lib/tabs/_tabs-common.scss

@@ -36,5 +36,5 @@ $md-tab-bar-height: 48px !default;
  position: absolute;
  bottom: 0;
  height: 2px;
-  transition: 350ms ease-out;
+  transition: 500ms ease-out;


Any reason not to use constants here?

hansl · 2016-11-09T18:43:41Z

src/lib/tabs/tab-group.scss

  display: flex;
+  transition: height 0.5s $ease-in-out-curve-function;


0.5s == 500ms; is this supposed to be the same measure as above for the ink-bar? If so, a constant would be even better :)

hansl · 2016-11-09T18:44:17Z

src/lib/tabs/tab-group.scss

-    display: block;
+md-tab-body {
+  display: block;
+  position: absolute;


I'm not 100% sure but I think we already have a mixin for "fullscreen" css. If not, we should probably have one somewhere.

I don't see that we have one already. Want me to make one in this change?

We do have the md-fullscreen mixin, which is I think what Hans means. But it is position: fixed, not absolute.

Right, there should probably be md-fill mixin or something like that with position: absolute. That mixin does not work right now.

hansl · 2016-11-09T19:28:48Z

src/lib/tabs/_tabs-common.scss

@@ -36,5 +36,5 @@ $md-tab-bar-height: 48px !default;
  position: absolute;
  bottom: 0;
  height: 2px;
-  transition: 350ms ease-out;
+  transition: 500ms ease-out;
 }


End of file, end of line.

hansl · 2016-11-09T19:28:55Z

src/lib/tabs/tab-body.html

+     (@position.start)="_onAnimationStarted($event)"
+     (@position.done)="_onAnimationComplete($event)">
+  <template portalHost></template>
+</div>


End of line.

hansl · 2016-11-09T19:38:33Z

src/lib/tabs/tabs.ts

+  templateUrl: 'tab-body.html',
+  animations: [
+    trigger('position', [
+      state('left', style({transform: 'translateX(-100%)'})),


According to https://jsperf.com/translate3d-vs-xy/28, translate3d is the fastest in most browsers (because 3d rendering), which is why we use it in most places.

Seems that Chrome has them set at about equal operations but 3D is way faster in Safari (and probably others). Thanks for the heads up

kara

Nits

kara · 2016-11-09T19:33:29Z

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

+
+    it('should update tab positions and attach content when selected', fakeAsync(() => {
+      fixture.detectChanges();
+      let tabComponent = fixture.debugElement.query(By.css('md-tab-group')).componentInstance;


Out of curiosity, why is this one let while the other is const? Seems like they could both be const.

Thanks, I tend to just default to let rather than const. Gotta break that habit.

kara · 2016-11-09T19:35:17Z

src/lib/tabs/tab-body.html

@@ -0,0 +1,6 @@
+<div class="md-tab-body-content"
+     [@position]="_position"


Nit: This animation name doesn't really describe what is happening with the animation.

kara · 2016-11-09T19:37:02Z

src/lib/tabs/tabs.ts

@@ -30,6 +37,7 @@ import {MdTabNavBar, MdTabLink} from './tab-nav-bar/tab-nav-bar';
 import {MdInkBar} from './ink-bar';
 import {Observable} from 'rxjs/Observable';
 import 'rxjs/add/operator/map';
+import {PortalHostDirective} from '../core/portal/portal-directives';


It might make more sense to add this to the existing import list on line 19 from ../core. I think it's exported there too.

kara · 2016-11-09T19:39:49Z

src/lib/tabs/tabs.ts

+   * Sets the height of the body wrapper to the height of the activating tab if dynamic
+   * height property is true.
+   */
+  _setTabBodyWrapperHeight(e: number) {


Nit: return type?

Maybe height would be more descriptive than e? It's not clear at first glance what it stands for.

kara · 2016-11-09T19:49:24Z

src/lib/tabs/tabs.ts

+    ])
+  ]
+})
+export class MdTabBody implements OnInit {


I'm curious why this has to be its own component. Is it for code org or is there another reason it has to be this way?

Primarily code organization - portal attach/detach + animations would have added quite a bit to tab group and I don't think it would have been as readable about what is happening between all the interactions

Okay, that's fair. Thought that might be the case.

andrewseguin · 2016-11-09T20:36:56Z

Changes made in response to the comments. Thanks for reviewing

kara

LGTM

andrewseguin · 2016-11-09T22:36:32Z

Switched md-fullscreen back to using absolute positioning (changed in #893) which is safe for its use in sidenav. Renamed as md-fill and used in the tab body.

andrewseguin · 2016-11-09T22:52:04Z

Sorry, added one more small change to support RTL animation direction

hansl · 2016-11-09T23:51:45Z

LGTM.

jelbourn

After this PR, we should make sure that the tabs accessibility has not changed, since there is now a state where the content of more than one tab is on-screen at once. We might need set aria-hidden on the tab that is being animated out.

jelbourn · 2016-11-11T02:25:59Z

e2e/components/tabs/tabs.e2e.ts

+/**
+ * Returns an array of true/false that represents the active states for the provided elements
+ * @param elements
+ * @returns {webdriver.promise.Promise<Promise<boolean>[]>|webdriver.promise.Promise<T[]>}


Since this is TypeScript, you don't need the type in the JsDoc (these were formerly js)

jelbourn · 2016-11-11T02:50:02Z

src/lib/tabs/tabs.ts

+    } else if (v > 0) {
+      this._position = this.getLayoutDirection() == 'ltr' ? 'right' : 'left';
+    }
+    if (v == 0) { this._position = 'center'; }


Just else here?

jelbourn · 2016-11-11T02:54:19Z

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

@@ -136,6 +139,53 @@ describe('MdTabGroup', () => {
      expect(component.handleSelection).toHaveBeenCalledTimes(1);
      expect(component.selectEvent.index).toBe(2);
    }));
+
+    it('should update tab positions and attach content when selected', fakeAsync(() => {


Also a test for RTL?

andrewseguin · 2016-11-11T17:27:27Z

Changes made (added test for RTL). Will make a note to check out screen reader support after this PR

angular-automatic-lock-bot · 2019-09-06T03:03:28Z

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.}

andrewseguin added the pr: needs review label Nov 9, 2016

andrewseguin assigned hansl, jelbourn and kara Nov 9, 2016

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 9, 2016

hansl suggested changes Nov 9, 2016

View reviewed changes

kara reviewed Nov 9, 2016

View reviewed changes

andrewseguin force-pushed the tabs-animate branch from 95e81f4 to 3067f8b Compare November 9, 2016 20:00

kara approved these changes Nov 9, 2016

View reviewed changes

kara added pr: lgtm and removed pr: needs review labels Nov 9, 2016

andrewseguin added 17 commits November 9, 2016 14:32

feat(tabs): add animations when switching tabs

9469bc5

add demo for extra content on a tab

8a47bcd

add flag for dynamic height

1424cbf

add scrolling when dynamic height is disabled

3021cc0

change style of imports on tabs

367532d

fix scrolling when switching away from a tab

958718c

fix tests

8db5984

add tests

40b5916

fix styling

e1863da

fix test

9fe9e7c

change demo styling

ed5c17e

fix lint

9c2bab1

fix e2e

7c283c8

responding to feedback

e41113f

responding to comments

a087c6e

repurpose md-fullscreen as md-fill for absolute

df0bba5

add extra line

54744b7

andrewseguin force-pushed the tabs-animate branch from 98df4f2 to 54744b7 Compare November 9, 2016 22:32

andrewseguin added 2 commits November 9, 2016 14:50

support rtl direction

338b94d

fix import

afd0ffb

hansl approved these changes Nov 9, 2016

View reviewed changes

jelbourn reviewed Nov 11, 2016

View reviewed changes

add rtl test

29533e7

jelbourn added the action: merge The PR is ready for merge by the caretaker label Nov 11, 2016

jelbourn merged commit f6944e4 into angular:master Nov 11, 2016

andrewseguin deleted the tabs-animate branch November 11, 2016 22:58

RobJacobs mentioned this pull request May 31, 2017

[Tabs] Md-tab height: 100% does not give all height of group #4591

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tabs): add animation when switching tabs, include optional dynamic height #1788

feat(tabs): add animation when switching tabs, include optional dynamic height #1788

andrewseguin commented Nov 9, 2016

hansl left a comment

hansl Nov 9, 2016

hansl Nov 9, 2016

hansl Nov 9, 2016

andrewseguin Nov 9, 2016

kara Nov 9, 2016 •

edited

Loading

hansl Nov 9, 2016

kara Nov 9, 2016

hansl Nov 9, 2016

hansl Nov 9, 2016

hansl Nov 9, 2016

andrewseguin Nov 9, 2016

kara left a comment

kara Nov 9, 2016

andrewseguin Nov 9, 2016

kara Nov 9, 2016

kara Nov 9, 2016

kara Nov 9, 2016

kara Nov 9, 2016

kara Nov 9, 2016

andrewseguin Nov 9, 2016

kara Nov 9, 2016

andrewseguin commented Nov 9, 2016

kara left a comment

andrewseguin commented Nov 9, 2016

andrewseguin commented Nov 9, 2016

hansl commented Nov 9, 2016

jelbourn left a comment

jelbourn Nov 11, 2016

jelbourn Nov 11, 2016

jelbourn Nov 11, 2016

andrewseguin commented Nov 11, 2016 •

edited

Loading

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

		display: flex;
		transition: height 0.5s $ease-in-out-curve-function;

		@@ -0,0 +1,6 @@
		<div class="md-tab-body-content"
		[@position]="_position"

feat(tabs): add animation when switching tabs, include optional dynamic height #1788

feat(tabs): add animation when switching tabs, include optional dynamic height #1788

Conversation

andrewseguin commented Nov 9, 2016

hansl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kara Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Nov 9, 2016

kara left a comment

Choose a reason for hiding this comment

andrewseguin commented Nov 9, 2016

andrewseguin commented Nov 9, 2016

hansl commented Nov 9, 2016

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Nov 11, 2016 • edited Loading

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

kara Nov 9, 2016 •

edited

Loading

andrewseguin commented Nov 11, 2016 •

edited

Loading