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

fix(carousel): respect the order of the slides #3416

Closed
wants to merge 1 commit into from
Closed

fix(carousel): respect the order of the slides #3416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 35 additions & 10 deletions src/carousel/carousel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
var destroyed = false;
/* direction: "prev" or "next" */
self.select = $scope.select = function(nextSlide, direction) {
var nextIndex = slides.indexOf(nextSlide);
var nextIndex = self.indexOfSlide(nextSlide);
//Decide direction if it's not given
if (direction === undefined) {
direction = nextIndex > currentIndex ? 'next' : 'prev';
direction = nextIndex > self.getCurrentIndex() ? 'next' : 'prev';
}
if (nextSlide && nextSlide !== self.currentSlide) {
goNext();
Expand All @@ -31,7 +31,6 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])

angular.extend(nextSlide, {direction: direction, active: true});
angular.extend(self.currentSlide || {}, {direction: direction, active: false});

if ($animate.enabled() && !$scope.noTransition && nextSlide.$element) {
$scope.$currentTransition = true;
// TODO: Switch to use .one when upgrading beyond 1.2.21
Expand All @@ -52,26 +51,45 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
destroyed = true;
});

function getSlideByIndex(index) {
if (angular.isUndefined(slides[index].index)) {
return slides[index];
}
var i, len = slides.length;
for (i = 0; i < slides.length; ++i) {
if (slides[i].index == index) {
return slides[i];
}
}
}

self.getCurrentIndex = function() {
if (self.currentSlide && angular.isDefined(self.currentSlide.index)) {
return +self.currentSlide.index;
}
return currentIndex;
};

/* Allow outside people to call indexOf on slides array */
self.indexOfSlide = function(slide) {
return slides.indexOf(slide);
return angular.isDefined(slide.index) ? +slide.index : slides.indexOf(slide);
};

$scope.next = function() {
var newIndex = (currentIndex + 1) % slides.length;
var newIndex = (self.getCurrentIndex() + 1) % slides.length;

//Prevent this user-triggered transition from occurring if there is already one in progress
if (!$scope.$currentTransition) {
return self.select(slides[newIndex], 'next');
return self.select(getSlideByIndex(newIndex), 'next');
}
};

$scope.prev = function() {
var newIndex = currentIndex - 1 < 0 ? slides.length - 1 : currentIndex - 1;
var newIndex = self.getCurrentIndex() - 1 < 0 ? slides.length - 1 : self.getCurrentIndex() - 1;

//Prevent this user-triggered transition from occurring if there is already one in progress
if (!$scope.$currentTransition) {
return self.select(slides[newIndex], 'prev');
return self.select(getSlideByIndex(newIndex), 'prev');
}
};

Expand Down Expand Up @@ -134,6 +152,11 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
};

self.removeSlide = function(slide) {
if (angular.isDefined(slide.index)) {
slides.sort(function(a, b) {
return +a.index > +b.index;
});
}
//get the index of the slide inside the carousel
var index = slides.indexOf(slide);
slides.splice(index, 1);
Expand Down Expand Up @@ -213,13 +236,14 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition'])
* Creates a slide inside a {@link ui.bootstrap.carousel.directive:carousel carousel}. Must be placed as a child of a carousel element.
*
* @param {boolean=} active Model binding, whether or not this slide is currently active.
* @param {number=} index The index of the slide. The slides will be sorted by this parameter.
*
* @example
<example module="ui.bootstrap">
<file name="index.html">
<div ng-controller="CarouselDemoCtrl">
<carousel>
<slide ng-repeat="slide in slides" active="slide.active">
<slide ng-repeat="slide in slides" active="slide.active" index="$index">
<img ng-src="{{slide.image}}" style="margin:auto;">
<div class="carousel-caption">
<h4>Slide {{$index}}</h4>
Expand Down Expand Up @@ -253,7 +277,8 @@ function CarouselDemoCtrl($scope) {
replace: true,
templateUrl: 'template/carousel/slide.html',
scope: {
active: '=?'
active: '=?',
index: '=?'
},
link: function (scope, element, attrs, carouselCtrl) {
carouselCtrl.addSlide(scope, element);
Expand Down
70 changes: 70 additions & 0 deletions src/carousel/test/carousel.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,76 @@ describe('carousel', function() {
expect($interval.cancel).toHaveBeenCalled();
});

describe('slide order', function() {

beforeEach(function() {
scope.slides = [
{active:false,content:'one', id:1},
{active:false,content:'two', id:2},
{active:false,content:'three', id:3}
];
elm = $compile(
'<carousel interval="interval" no-transition="true" no-pause="nopause">' +
'<slide ng-repeat="slide in slides | orderBy: \'id\' " active="slide.active" index="$index">' +
'{{slide.content}}' +
'</slide>' +
'</carousel>'
)(scope);
scope.$apply();
scope.slides[0].id = 3;
scope.slides[1].id = 1;
scope.slides[2].id = 2;
scope.$apply();
});

it('should change dom when an order of the slides was changed', function() {
testSlideActive(0);
var contents = elm.find('div.item');
expect(contents.length).toBe(3);
expect(contents.eq(0).text()).toBe('two');
expect(contents.eq(1).text()).toBe('three');
expect(contents.eq(2).text()).toBe('one');
});

it('should select next after order change', function() {
testSlideActive(0);
var next = elm.find('a.right');
next.click();
testSlideActive(1);
});

it('should select prev after order change', function() {
testSlideActive(0);
var prev = elm.find('a.left');
prev.click();
testSlideActive(2);
});

it('should add slide in the specified position', function() {
testSlideActive(0);
scope.slides[2].id = 4;
scope.slides.push({active:false,content:'four', id:2});
scope.$apply();
var contents = elm.find('div.item');
expect(contents.length).toBe(4);
expect(contents.eq(0).text()).toBe('two');
expect(contents.eq(1).text()).toBe('four');
expect(contents.eq(2).text()).toBe('one');
expect(contents.eq(3).text()).toBe('three');
});

it('should remove slide after order change', function() {
testSlideActive(0);
scope.slides.splice(1, 1);
scope.$apply();
var contents = elm.find('div.item');
expect(contents.length).toBe(2);
expect(contents.eq(0).text()).toBe('three');
expect(contents.eq(1).text()).toBe('one');
});

});

});

describe('controller', function() {
Expand Down
2 changes: 1 addition & 1 deletion template/carousel/carousel.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<div ng-mouseenter="pause()" ng-mouseleave="play()" class="carousel" ng-swipe-right="prev()" ng-swipe-left="next()">
<ol class="carousel-indicators" ng-show="slides.length > 1">
<li ng-repeat="slide in slides track by $index" ng-class="{active: isActive(slide)}" ng-click="select(slide)"></li>
<li ng-repeat="slide in slides | orderBy:'index' track by $index" ng-class="{active: isActive(slide)}" ng-click="select(slide)"></li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you extend this so that it can support functions as well? It should support all of the possibilities of orderBy IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I'm not sure that I understand what you mean. In this peace of code we're just ordering the carousel indicators by the slide index parameter. Do you mean that we should do something like this:

<li ng-repeat="slide in slides | orderBy:orderIndicators track by $index" ng-class="{active: isActive(slide)}" ng-click="select(slide)"></li>

and then in controller:

$scope.orderIndicators = function(slide) {
    return slide.index;
};

What's the purpose of such extension?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my concern is if the user is doing something like

<slide ng-repeat="slide in slides | orderBy: customOrderFn">
...
</slide>

I'm not sure whether that custom order would be respected by this implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be, please check this plunker: http://plnkr.co/edit/pmCFIH0GCpBjNBWY8g1D?p=preview.

</ol>
<div class="carousel-inner" ng-transclude></div>
<a class="left carousel-control" ng-click="prev()" ng-show="slides.length > 1"><span class="glyphicon glyphicon-chevron-left"></span></a>
Expand Down