feat(tabs): add swipe events #6294

amcdnl · 2017-08-05T17:05:31Z

Addresses #2209 by adding swipe events to the tab content area.

jelbourn

@amcdnl trying this out, it seems like this breaks the ability to scroll the tab content by dragging on mobile
(I just tried on the device emulator in Chrome)

jelbourn · 2017-08-08T18:56:32Z

src/lib/core/gestures/gesture-annotations.ts

@@ -54,3 +54,12 @@ export interface HammerManager {
  off(events: string, handler?: Function): void;
  on(events: string, handler: Function): void;
 }
+
+/** @docs-private */
+export interface HammerEvent {


Why introduce this instead of using the existing HammerInput type?

Did not realize those were the same, I'll consolidate.

jelbourn · 2017-08-08T19:01:42Z

src/lib/tabs/tab-group.ts

+  _bodyContentSwiped(event: HammerEvent): void {
+    if(this.selectedIndex === null) this.selectedIndex = 0;
+    if(this.selectedIndex !== 0 && event.type === 'swiperight') {
+      this.selectedIndex = this._findNextTabByIndex(this.selectedIndex, 'left');


No need to send left or right instead of directly sending the delta

jelbourn · 2017-08-08T19:03:02Z

src/lib/tabs/tab-body.html

@@ -1,4 +1,6 @@
 <div class="mat-tab-body-content" #content
+     (swipeleft)="onSwipe.emit($event)"
+     (swiperight)="onSwipe.emit($event)"


Not just (swipe)?

jelbourn · 2017-08-08T19:03:41Z

src/lib/tabs/tab-group.ts

+    if(this.selectedIndex !== 0 && event.type === 'swiperight') {
+      this.selectedIndex = this._findNextTabByIndex(this.selectedIndex, 'left');
+    } else if(this.selectedIndex < this._tabs.length && event.type === 'swipeleft') {
+      this.selectedIndex = this._findNextTabByIndex(this.selectedIndex, 'right');


Are left and right reversed in RTL?

jelbourn · 2017-08-08T19:05:56Z

src/lib/tabs/tab-header.html

@@ -7,6 +7,8 @@
 </div>

 <div class="mat-tab-label-container" #tabListContainer
+     (swipeleft)="_handleSwipe($event)"
+     (swiperight)="_handleSwipe($event)"


This captures the swipe gesture specifically, but it feels like the header would need to capture dragging as well (which I think Hammer captures under panmove)

Good idea. It has a drag property I listen to.

jelbourn · 2017-08-08T19:14:08Z

src/lib/tabs/tab-group.ts

+
+  /** Body content was swiped left/right */
+  _bodyContentSwiped(event: HammerEvent): void {
+    if(this.selectedIndex === null) this.selectedIndex = 0;


Braces are always required

jelbourn · 2017-08-08T19:14:23Z

src/lib/tabs/tab-group.ts

+    const tabsArray = this._tabs.toArray();
+    const nextIdx = direction === 'right' ? index + 1 : index - 1;
+
+    if(nextIdx < tabsArray.length && nextIdx >= 0) {


Space between if and paren (here and elsewhere)

jelbourn · 2017-08-08T22:08:01Z

src/lib/tabs/tab-group.ts

@@ -275,4 +275,31 @@ export class MdTabGroup extends _MdTabGroupMixinBase implements AfterContentInit
    this._tabBodyWrapperHeight = this._tabBodyWrapper.nativeElement.clientHeight;
    this._renderer.setStyle(this._tabBodyWrapper.nativeElement, 'height', '');
  }
+
+  /** Finds the next tab in the stack for the given direction */
+  _findNextTabByIndex(index: number, direction: string): number {


Similar function elsewhere in library: https://github.com/angular/material2/blob/master/src/cdk/a11y/list-key-manager.ts#L203

amcdnl · 2017-08-12T14:42:39Z

@jelbourn - I'm getting a lint error saying:

ERROR: /Users/austin/dev/material2/src/lib/tabs/tab-body.ts[30, 73]: 'HammerInput' is declared but never used.

which is false because its being used in the onSwipe event here:

@Output() onSwipe: EventEmitter<HammerInput> = new EventEmitter<HammerInput>();

amcdnl · 2017-08-13T15:42:39Z

@jelbourn - ready for review.

jelbourn

For the lint error: it's a bug in the version of TypeScript we use. Just need to suppress it until we bump the TS version

Seems good overall, just needs unit tests (the slider has examples of faking hammer events)

jelbourn · 2017-08-14T18:42:16Z

src/lib/tabs/tab-body.html

@@ -1,4 +1,6 @@
 <div class="mat-tab-body-content" #content
+     (swipe)="onSwipe.emit($event)"
+     [style.touch-action]="'pan-y'"


Add a comment for why this needs to be an inline style? Also, since this is static, you don't need a binding; just style="touch-action: pan-y" is good.

Would you like the comment to be a HTML inline comment?

This actually doesn't work if you do it static like you suggested. I had tried that at first implementation.

Reference: hammerjs/hammer.js#1014

I spent a bit too much time tracking this down. touch-action: none is coming from our material GestureConfig; adding the swipe gesture always sets the touch action to pan-y + pan-x (equivalent to none).

The "correct" fix for this would be to change how our gesture config add recognizers for certain events, but that's more complicated than we want to do right now. I filed #6484 to track this. Can you make your comment something like



Comment can go above the element (I don't think a comment inside of a tag is valid html). HTML comments are stripped when packaging the release

jelbourn · 2017-08-14T18:44:12Z

src/lib/tabs/tab-group.ts

+      this.selectedIndex = 0;
+    }
+
+    if (event.direction === 2 || event.direction === 4) {


Add an enum or constants instead of using the number literals in the function body

commit 85a6fff Author: mmalerba <mmalerba@google.com> Date: Sat Aug 12 15:05:58 2017 -0700 fix demo app (angular#6437)

… direction

amcdnl · 2017-08-15T22:52:30Z

@jelbourn - added unit tests and comments, ready for review again.

rafaelss95 · 2017-08-15T23:53:44Z

@jelbourn I just searched "enum" in repo and found this (PascalCased enum members) and this (UPPERCASED enum members).

... and the enum members here (from HammerDirection) are going to be lowercased. I'm not sure what's the standard for enums in the repo, but I think the case of enum members should be consistent, shouldn't it?

Note that according this style-guide the recommended approach for enum members is to use PascalCase.

jelbourn · 2017-08-16T16:32:56Z

src/lib/core/gestures/gesture-annotations.ts

  deltaY: number;
  center: { x: number; y: number; };
 }

 /** @docs-private */
+export enum HammerDirection {
+  left = 2,
+  right = 4


Enum fields should be PascalCase

jelbourn · 2017-08-16T16:38:08Z

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

@@ -214,6 +219,21 @@ describe('MdTabHeader', () => {
          .toBe(0, 'Expected no ripple to show up after mousedown');
      });

+      it('should update scroll to next set on swipe', () => {


jelbourn · 2017-08-16T16:38:51Z

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

+      const body =  fixture.nativeElement.
+        querySelector('.mat-tab-body-active .mat-tab-body-content');
+
+      dispatchSwipeEvent(body, 2, gestureConfig);


Use the enum rather than hard-coded numbers

jelbourn · 2017-08-16T16:39:53Z

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

+
+      // select the second tab
+      const body =  fixture.nativeElement.
+        querySelector('.mat-tab-body-active .mat-tab-body-content');


Prefer breaking at the higher syntactic level:

const body = fixture.nativeElement.querySelector('.mat-tab-body-active .mat-tab-body-content');

jelbourn · 2017-08-16T16:40:59Z

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

@@ -169,6 +176,22 @@ describe('MdTabGroup', () => {
      expect(testElement.querySelectorAll('.mat-ripple-element').length)
        .toBe(0, 'Expected no ripple to show up on label mousedown.');
    });
+
+    it('should switch tabs on body swipe', () => {


Also need tests for skipping disabled tabs and for swipes at the first/last tab.

jelbourn · 2017-08-16T16:41:10Z

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

+    config.emitEventForElement('swipe', bodyElement, {
+      deltaX: direction === 2 ? -100 : 100,
+      direction: direction,
+      srcEvent: { preventDefault: jasmine.createSpy('preventDefault') }


Omit spaces in braces

jelbourn · 2017-08-16T16:43:33Z

src/lib/tabs/tab-group.ts

+      let direction = event.direction;
+      if (this._dir.value === 'rtl') {
+        direction = direction === HammerDirection.left ?
+          HammerDirection.right : HammerDirection.left;


Overflow indent is +4, but this one can also be broken at a higher syntactic level

direction = direction === HammerDirection.left ? HammerDirection.right : HammerDirection.left;

…-swipe

amcdnl · 2017-08-16T21:43:50Z

@jelbourn - rdy to go.

jelbourn

LGTM, @andrewseguin should also take a look

andrewseguin · 2017-08-22T21:56:54Z

Looks good to me as well, thanks for adding this!

jelbourn · 2018-06-15T17:00:33Z

Maybe a flake? Try rebasing and re-pushing?

CharlBest · 2018-06-20T11:01:06Z

I was just wondering how it is going with that weird error? Did a little search and found this. Hope it helps. microsoft/TypeScript#14538 (comment)

CharlBest · 2018-07-12T07:49:16Z

Hi @amcdnl do you think this PR will make the next minor release?

amcdnl · 2018-07-15T19:13:45Z

@CharlBest - That would be a @jelbourn call.

jelbourn · 2018-07-16T19:04:58Z

Looks like there are still failures on CI

CharlBest · 2018-07-19T07:34:48Z

@amcdnl Do you think the CI failures are difficult to fix?

CharlBest · 2018-07-31T09:22:54Z

Any progress on this?

CharlBest · 2018-08-09T08:41:39Z

Any progress on this @amcdnl ?

ngbot · 2018-08-20T20:44:44Z

Hi @amcdnl! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-08-20T20:45:49Z

Hi @amcdnl! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

CharlBest · 2018-08-23T08:05:18Z

@amcdnl do you think this will make the v7 release?

CharlBest · 2018-08-30T07:48:16Z

@jelbourn Do you think this PR will be considered or is it out of scope at the moment?

jelbourn · 2018-08-30T14:05:12Z

Austin has been taking some time off, we'll revisit when he gets back

CharlBest · 2018-08-30T15:01:00Z

@jelbourn Thank you for the reply. Good luck with everything you're working on and great job.

CharlBest · 2018-10-04T08:34:01Z

@jelbourn It doesn't look like @amcdnl will be working on this or am I wrong? Can I help with this PR? Just think this will give the tabs a nice native feeling.

andrewseguin · 2019-02-14T19:21:31Z

@CharlBest If you would like to take over on this pull request, then I'd be happy to work with you on reviewing and getting it merged in

CharlBest · 2019-02-19T13:51:25Z

Hi @andrewseguin

Yes sure. I won't mind doing it although I'm not exactly sure what I'm suppose to do. :)

ceolinrenato · 2019-05-23T20:06:49Z

@andrewseguin @CharlBest did you guys took over this pull request? Or is it still stalled?
I'm highly interested in this feature

jelbourn · 2019-05-24T16:17:32Z

We decided recently that we want to remove HammerJS as a dependency, so we probably won't add this directly to the library any more, and instead document instructions for how to accomplish it with the gesture library of your choice.

jelbourn · 2019-06-13T17:50:48Z

Closing this since we've decided to remove Hammer as a dependency and push gesture recognition to the application layer

angular-automatic-lock-bot · 2019-09-10T23:02:50Z

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

feat(tabs): add swipe events angular#2209

9c7f20b

amcdnl requested a review from jelbourn August 5, 2017 17:05

amcdnl requested a review from andrewseguin as a code owner August 5, 2017 17:05

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

feat(tabs): handle swipes in tab nav

7f2ff6f

jelbourn added the pr: needs review label Aug 8, 2017

jelbourn reviewed Aug 8, 2017

View reviewed changes

amcdnl added 3 commits August 12, 2017 10:21

feat(tabs): rtl, drag events, event consolidation

cb40cc8

Merge branch 'master' into tab-swipe

aaa6cef

feat(tabs): fix pan-y not working in tab gestures

9c84af8

jelbourn reviewed Aug 14, 2017

View reviewed changes

amcdnl added 3 commits August 14, 2017 17:33

Squashed commit of the following:

c2ce180

commit 85a6fff Author: mmalerba <mmalerba@google.com> Date: Sat Aug 12 15:05:58 2017 -0700 fix demo app (angular#6437)

tests(tabs): add gesture tests, refactor gesture script, add enum for…

43282e7

… direction

chore(*): adding workaround comment

33d584e

amcdnl requested review from devversion and mmalerba as code owners August 15, 2017 22:50

jelbourn reviewed Aug 16, 2017

View reviewed changes

amcdnl added 2 commits August 16, 2017 16:41

feat(tabs): nit

897275a

Merge branch 'master' of https://github.com/amcdnl/material2 into tab…

75ffcc2

…-swipe

amcdnl force-pushed the tab-swipe branch from ac751cf to 75ffcc2 Compare August 16, 2017 21:42

Merge branch 'master' into tab-swipe

5557b80

jelbourn approved these changes Aug 17, 2017

View reviewed changes

jelbourn assigned andrewseguin Aug 17, 2017

amcdnl mentioned this pull request Aug 21, 2017

Tab header swipe #2291

Closed

Merge branch 'master' into tab-swipe

acb5525

josephperrott added target: minor This PR is targeted for the next minor release and removed action: merge The PR is ready for merge by the caretaker labels Jun 26, 2018

Merge remote-tracking branch 'upstream/master' into tab-swipe

d68f2f0

Merge branch 'master' into tab-swipe

52c5b4e

mmalerba added aaa and removed aaa labels Apr 25, 2019

jelbourn closed this Jun 13, 2019

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

feat(tabs): add swipe events #6294

feat(tabs): add swipe events #6294

Conversation

amcdnl commented Aug 5, 2017

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

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

amcdnl commented Aug 12, 2017

amcdnl commented Aug 13, 2017

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcdnl Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcdnl commented Aug 15, 2017

rafaelss95 commented Aug 15, 2017 • 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

amcdnl commented Aug 16, 2017

jelbourn left a comment • edited Loading

Choose a reason for hiding this comment

andrewseguin commented Aug 22, 2017

jelbourn commented Jun 15, 2018

CharlBest commented Jun 20, 2018

CharlBest commented Jul 12, 2018

amcdnl commented Jul 15, 2018

jelbourn commented Jul 16, 2018

CharlBest commented Jul 19, 2018

CharlBest commented Jul 31, 2018

CharlBest commented Aug 9, 2018

ngbot bot commented Aug 20, 2018

ngbot bot commented Aug 20, 2018

CharlBest commented Aug 23, 2018

CharlBest commented Aug 30, 2018

jelbourn commented Aug 30, 2018

CharlBest commented Aug 30, 2018

CharlBest commented Oct 4, 2018

andrewseguin commented Feb 14, 2019

CharlBest commented Feb 19, 2019 • edited Loading

ceolinrenato commented May 23, 2019

jelbourn commented May 24, 2019

jelbourn commented Jun 13, 2019

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

amcdnl Aug 14, 2017 •

edited

Loading

rafaelss95 commented Aug 15, 2017 •

edited

Loading

jelbourn left a comment •

edited

Loading

CharlBest commented Feb 19, 2019 •

edited

Loading