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

fix(carousel): re-enable deprecated directives #4527

Closed
wants to merge 1 commit into from
Closed

fix(carousel): re-enable deprecated directives #4527

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
31 changes: 27 additions & 4 deletions src/carousel/carousel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,6 @@ angular.module('ui.bootstrap.carousel', [])
*/
.directive('uibCarousel', [function() {
return {
restrict: 'EA',
transclude: true,
replace: true,
controller: 'UibCarouselController',
Expand Down Expand Up @@ -421,11 +420,24 @@ angular.module('ui.bootstrap.carousel')

.value('$carouselSuppressWarning', false)

.controller('CarouselController', ['$scope', '$element', '$controller', '$log', '$carouselSuppressWarning', function($scope, $element, $controller, $log, $carouselSuppressWarning) {
if (!$carouselSuppressWarning) {
$log.warn('CarouselController is now deprecated. Use UibCarouselController instead.');
}

return $controller('UibCarouselController', {
$scope: $scope,
$element: $element
});
}])

.directive('carousel', ['$log', '$carouselSuppressWarning', function($log, $carouselSuppressWarning) {
return {
restrict: 'EA',
transclude: true,
replace: true,
controller: 'CarouselController',
controllerAs: 'carousel',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed? I don't see any usage of scope.carousel .

Copy link
Contributor

Choose a reason for hiding this comment

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

We started to add this option to some directives to expose the controller to the template using controllerAs. It is not something I would add personally, but there it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.. okay, sounds like a separate feature though. But okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excuse me Chris, but after a couple of months away of the project, there is lots of stuff being done and I am afraid that you're barking to the wrong tree. I totally understand that.

Imagine that by mistake, we make a commit where we delete an entire directive. Then we want to create another PR to put back the content we deleted. We are simply doing a chore to put it back like it was before. We are not creating multiple PRs with all the features we deleted one by one. We are just restoring something that was there in a first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. It was added as a feature in #4131, I wasn't aware of that.

require: 'carousel',
templateUrl: function(element, attrs) {
return attrs.templateUrl || 'template/carousel/carousel.html';
},
Expand All @@ -445,8 +457,7 @@ angular.module('ui.bootstrap.carousel')

.directive('slide', ['$log', '$carouselSuppressWarning', function($log, $carouselSuppressWarning) {
return {

restrict: 'EA',
require: '^carousel',
transclude: true,
replace: true,
templateUrl: function(element, attrs) {
Expand All @@ -461,6 +472,18 @@ angular.module('ui.bootstrap.carousel')
if (!$carouselSuppressWarning) {
$log.warn('slide is now deprecated. Use uib-slide instead.');
}

carouselCtrl.addSlide(scope, element);
//when the scope is destroyed then remove the slide from the current slides array
scope.$on('$destroy', function() {
carouselCtrl.removeSlide(scope);
});

scope.$watch('active', function(active) {
if (active) {
carouselCtrl.select(scope);
}
});
}
};
}]);
24 changes: 13 additions & 11 deletions src/carousel/test/carousel.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,6 @@ describe('carousel', function() {
});

describe('slide order', function() {

beforeEach(function() {
scope.slides = [
{active:false,content:'one', id:1},
Expand Down Expand Up @@ -551,27 +550,30 @@ describe('carousel deprecation', function() {
spyOn($log, 'warn');

var element = '<carousel interval="interval" no-transition="true" no-pause="nopause">' +
'<slide ng-repeat="slide in slides" active="slide.active">' +
'{{slide.content}}' +
'</slide>' +
'</carousel>';
'<slide ng-repeat="slide in slides" active="slide.active">' +
'{{slide.content}}' +
'</slide>' +
'</carousel>';
element = $compile(element)($rootScope);
$rootScope.$digest();

expect($log.warn.calls.count()).toBe(1);
expect($log.warn.calls.argsFor(0)).toEqual(['carousel is now deprecated. Use uib-carousel instead.']);
expect($log.warn.calls.count()).toBe(2);
expect($log.warn.calls.argsFor(0)).toEqual(['CarouselController is now deprecated. Use UibCarouselController instead.']);
expect($log.warn.calls.argsFor(1)).toEqual(['carousel is now deprecated. Use uib-carousel instead.']);
}));

it('should give warning by default for slider', inject(function($compile, $log, $rootScope) {
spyOn($log, 'warn');

var element = '<carousel interval="interval" no-transition="true" no-pause="nopause">' +
'<slide></slide>' +
'</carousel>';
'<slide></slide>' +
'</carousel>';
element = $compile(element)($rootScope);
$rootScope.$digest();

expect($log.warn.calls.count()).toBe(2);
expect($log.warn.calls.argsFor(0)).toEqual(['slide is now deprecated. Use uib-slide instead.']);
expect($log.warn.calls.count()).toBe(3);
expect($log.warn.calls.argsFor(0)).toEqual(['CarouselController is now deprecated. Use UibCarouselController instead.']);
expect($log.warn.calls.argsFor(1)).toEqual(['slide is now deprecated. Use uib-slide instead.']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Check all messages.

expect($log.warn.calls.argsFor(2)).toEqual(['carousel is now deprecated. Use uib-carousel instead.']);
}));
});