Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Commit

Permalink
fix(carousel): delay select until $digest stabilizes
Browse files Browse the repository at this point in the history
- Defer selection of active slide until after transition has been reset
  to avoid selection occuring before the transition finishes, preventing
a new slide from being chosen
- Buffer slides if transition is currently going on to navigate to last
  slide selected after
  • Loading branch information
wesleycho committed Dec 1, 2015
1 parent a8b0d45 commit ace3787
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 13 deletions.
49 changes: 39 additions & 10 deletions src/carousel/carousel.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
angular.module('ui.bootstrap.carousel', [])

.controller('UibCarouselController', ['$scope', '$element', '$interval', '$animate', function($scope, $element, $interval, $animate) {
.controller('UibCarouselController', ['$scope', '$element', '$interval', '$timeout', '$animate', function($scope, $element, $interval, $timeout, $animate) {
var self = this,
slides = self.slides = $scope.slides = [],
SLIDE_DIRECTION = 'uib-slideDirection',
currentIndex = -1,
currentInterval, isPlaying;
currentInterval, isPlaying, bufferedTransitions = [];
self.currentSlide = null;

var destroyed = false;
Expand All @@ -19,6 +19,8 @@ angular.module('ui.bootstrap.carousel', [])
//Prevent this user-triggered transition from occurring if there is already one in progress
if (nextSlide && nextSlide !== self.currentSlide && !$scope.$currentTransition) {
goNext(nextSlide, nextIndex, direction);
} else if (nextSlide && nextSlide !== self.currentSlide && $scope.$currentTransition) {
bufferedTransitions.push(nextSlide);
}
};

Expand All @@ -40,6 +42,14 @@ angular.module('ui.bootstrap.carousel', [])
if (phase === 'close') {
$scope.$currentTransition = null;
$animate.off('addClass', element);
if (bufferedTransitions.length) {
var nextSlide = bufferedTransitions.pop();
var nextIndex = $scope.indexOfSlide(nextSlide);
var nextDirection = nextIndex > self.getCurrentIndex() ? 'next' : 'prev';
clearBufferedTransitions();

goNext(nextSlide, nextIndex, nextDirection);
}
}
});
}
Expand Down Expand Up @@ -134,6 +144,13 @@ angular.module('ui.bootstrap.carousel', [])
function resetTransition(slides) {
if (!slides.length) {
$scope.$currentTransition = null;
clearBufferedTransitions();
}
}

function clearBufferedTransitions() {
while (bufferedTransitions.length) {
bufferedTransitions.shift();
}
}

Expand All @@ -155,6 +172,10 @@ angular.module('ui.bootstrap.carousel', [])
slides.push(slide);
//if this is the first slide or the slide is set to active, select it
if (slides.length === 1 || slide.active) {
if ($scope.$currentTransition) {
$scope.$currentTransition = null;
}

self.select(slides[slides.length - 1]);
if (slides.length === 1) {
$scope.play();
Expand All @@ -170,22 +191,30 @@ angular.module('ui.bootstrap.carousel', [])
return +a.index > +b.index;
});
}

var bufferedIndex = bufferedTransitions.indexOf(slide);
if (bufferedIndex !== -1) {
bufferedTransitions.splice(bufferedIndex, 1);
}
//get the index of the slide inside the carousel
var index = slides.indexOf(slide);
slides.splice(index, 1);
if (slides.length > 0 && slide.active) {
if (index >= slides.length) {
self.select(slides[index - 1]);
} else {
self.select(slides[index]);
$timeout(function() {
if (slides.length > 0 && slide.active) {
if (index >= slides.length) {
self.select(slides[index - 1]);
} else {
self.select(slides[index]);
}
} else if (currentIndex > index) {
currentIndex--;
}
} else if (currentIndex > index) {
currentIndex--;
}
});

//clean the currentSlide when no more slide
if (slides.length === 0) {
self.currentSlide = null;
clearBufferedTransitions();
}
};

Expand Down
47 changes: 44 additions & 3 deletions src/carousel/test/carousel.spec.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
describe('carousel', function() {
fdescribe('carousel', function() {
beforeEach(module('ui.bootstrap.carousel', function($compileProvider, $provide) {
angular.forEach(['ngSwipeLeft', 'ngSwipeRight'], makeMock);
function makeMock(name) {
Expand All @@ -12,15 +12,18 @@ describe('carousel', function() {
});
}
}));
beforeEach(module('ngAnimateMock'));
beforeEach(module('uib/template/carousel/carousel.html', 'uib/template/carousel/slide.html'));

var $rootScope, $compile, $controller, $interval, $templateCache;
beforeEach(inject(function(_$rootScope_, _$compile_, _$controller_, _$interval_, _$templateCache_) {
var $rootScope, $compile, $controller, $interval, $templateCache, $timeout, $animate;
beforeEach(inject(function(_$rootScope_, _$compile_, _$controller_, _$interval_, _$templateCache_, _$timeout_, _$animate_) {
$rootScope = _$rootScope_;
$compile = _$compile_;
$controller = _$controller_;
$interval = _$interval_;
$templateCache = _$templateCache_;
$timeout = _$timeout_;
$animate = _$animate_;
}));

describe('basics', function() {
Expand Down Expand Up @@ -291,11 +294,13 @@ describe('carousel', function() {
scope.$apply('slides[2].active = true');
testSlideActive(2);
scope.$apply('slides.splice(0,1)');
$timeout.flush(0);
expect(elm.find('div.item').length).toBe(2);
testSlideActive(1);
$interval.flush(scope.interval);
testSlideActive(0);
scope.$apply('slides.splice(1,1)');
$timeout.flush(0);
expect(elm.find('div.item').length).toBe(1);
testSlideActive(0);
});
Expand Down Expand Up @@ -325,6 +330,39 @@ describe('carousel', function() {
testSlideActive(1);
});

it('should buffer the slides if transition is clicked and only transition to the last requested', function() {
var carouselScope = elm.children().scope();

testSlideActive(0);
carouselScope.$currentTransition = null;
carouselScope.select(carouselScope.slides[1]);
$animate.flush();

testSlideActive(1);

carouselScope.$currentTransition = true;
carouselScope.select(carouselScope.slides[2]);
scope.$apply();

testSlideActive(1);

carouselScope.select(carouselScope.slides[0]);
scope.$apply();

testSlideActive(1);

carouselScope.$currentTransition = null;
$interval.flush(scope.interval);
$animate.flush();

testSlideActive(2);

$interval.flush(scope.interval);
$animate.flush();

testSlideActive(0);
});

it('issue 1414 - should not continue running timers after scope is destroyed', function() {
testSlideActive(0);
$interval.flush(scope.interval);
Expand Down Expand Up @@ -468,13 +506,16 @@ describe('carousel', function() {
it('should remove slide and change active slide if needed', function() {
expect(ctrl.slides.length).toBe(4);
ctrl.removeSlide(ctrl.slides[0]);
$timeout.flush(0);
expect(ctrl.slides.length).toBe(3);
expect(ctrl.currentSlide).toBe(ctrl.slides[0]);
ctrl.select(ctrl.slides[2]);
ctrl.removeSlide(ctrl.slides[2]);
$timeout.flush(0);
expect(ctrl.slides.length).toBe(2);
expect(ctrl.currentSlide).toBe(ctrl.slides[1]);
ctrl.removeSlide(ctrl.slides[0]);
$timeout.flush(0);
expect(ctrl.slides.length).toBe(1);
expect(ctrl.currentSlide).toBe(ctrl.slides[0]);
});
Expand Down

0 comments on commit ace3787

Please sign in to comment.