feat(carousel): Support swipe for touchscreen devices

[mvhecke]

Allows users on touchscreen devices to swipe to next and previous images in the carousel.

[mvhecke]

Would offer support for #1416.

[Foxandxss]

Touch support is absolutely awesome, but that is adding a new dep so I think we should think about our goals about touch stuff.

[mvhecke]

@Foxandxss A solution would be to leave it up to people themselves whether or not they want to add touch support. Because when you don't load ngTouch the used directives will just become 'dumb' attribute. Though there still is some small overhead of the unused attributes in the template that way.

[Foxandxss]

I could live wit that tbh, is the cleanest way I think.

[ajoslin]

👍

The main thing I would do is simplify the tests. Right now you are more testing that the ngSwipe* directives work than anything, which makes complicated tests. If the logic of swiping changes your tests will fail. Also it has difference in browsers like you said.

I would instead make mock versions of ngSwipeLeft and ngSwipeRight that are simple and easy to test.

1. Erase existing ngSwipeLeftDirective if it exists
2. Create a new mock one.
3. Test against the mock directive

describe('carousel swiping', function() {
  beforeEach(module('ui.bootstrap.carousel', function($compileProvider, $provide) {
    angular.forEach(['ngSwipeLeft', 'ngSwipeRight', makeMock);
    function makeMock(name) {
      $provide.value(name + 'Directive', []); //remove existing directive if it exists
      $compileProvider.directive(name, function() {
        return function(scope, element, attr) {
          element.on(name, function() {
            scope.$apply(attr[name]);
          });
        };
      });
    }
  }));

  //..other stuff to setup

  it('should go next on swipeLeft', function() {
    elm.triggerHandler('ngSwipeLeft');
    //expectNextSlideToBeActive();
  });
});

[mvhecke]

@Foxandxss I will change it as we discussed.

@ajoslin It's true I borrowed a part of the test from Angular itself. I like what you're proposing as it's more future proof if things might change as you mentioned.

[mvhecke]

@ajoslin I can't seem to get it working, I've included it inside the describe('carousel', function() {. It only says the test failed.

beforeEach(module('ui.bootstrap.carousel', function($compileProvider, $provide) {
    angular.forEach(['ngSwipeLeft', 'ngSwipeRight'], makeMock);
    function makeMock(name) {
      $provide.value(name + 'Directive', []); //remove existing directive if it exists
      $compileProvider.directive(name, function() {
        return function(scope, element, attr) {
          element.on(name, function() {
            scope.$apply(attr[name]);
          });
        };
      });
    }
  }));

describe('swiping', function() {
      it('should go next on swipeLeft', function() {
        testSlideActive(0);
        elm.triggerHandler('ngSwipeLeft');
        testSlideActive(1);
      });

      it('should go prev on swipeRight', function() {
        testSlideActive(0);
        elm.triggerHandler('ngSwipeRight');
        testSlideActive(2);
      });
    });

[ajoslin]

hmm.. add some console.log in the fake directive (linking function and
inside the event) and see if anything is happening.

[mvhecke]

@ajoslin It's one of those days that you don't change a thing but it all of a sudden works!

[mvhecke]

What do you guys think about the documentation for this? Don't mind the strange commit, I will rebase it later.

[ajoslin]

Looks good.

Maybe just add a sentence like 'The carousel responds to clicks and swipes.'

[mvhecke]

I kinda made a mess of it and I can't seem to get everything back to a single commit.

[ajoslin]

git rebase -i HEAD~6 - brings up last 6 commits for interactive rebasing in your text editor.

Now you want to 'squash' all of the commits after feat(carousel): Support swipe for touchscreen devices into one. Just put squash before all the commits after the one you want to put together.

Now save and quit, and go through the screens it gives you and it will squash your commits together.

[mvhecke]

Ok, I've squashed them all into one but when pushing I run into a non-fast-forward.

[ajoslin]

So now you've modified your local repository in a way that the remote cannot recognize - because you "changed the history" of commits - erasing commits.

This is OK to do on a development branch, but not good to do on master.

You will want to do git push -f origin carousel-swipe to force-push your history rewrite to the server.

[mvhecke]

Everything should be fine now, I've also added some explanation about enabling touch. Thanks @ajoslin! I'm not really familiar with Git, which I hope to improve in the future.

[Foxandxss]

I Like it :)

[ajoslin]

Pulled it down and changed a couple things:

• Removed ngTouch dependency for tests and in test-lib, as it wasn't being used
• Added ngTouch dependency to demo/asses/app.js so the demo has swipe support

Please be sure to test that the demo page works as expected in the future.

Merged now. Thanks @Gamemaniak and please keep contributing!

[mvhecke]

@ajoslin Things got a bit chaotic due to the rebasing I think. Of course I will keep contributing!

[ajoslin]

👍 Open source is the bomb. Random people helping random people so everyone can make good things!