feat(carousel): adding noWrap option to prevent re-cycling of slides #3462
Conversation
| @@ -219,7 +223,8 @@ angular.module('ui.bootstrap.carousel', []) | |||
| scope: { | |||
| interval: '=', | |||
| noTransition: '=', | |||
| noPause: '=' | |||
| noPause: '=', | |||
| noWrap: '=?' | |||
There was a problem hiding this comment.
Since this is readonly, use '&' instead and read it using $scope.noWrap(). This saves on a watcher.
We'll eventually change the rest of these scope variables to use '&' for performance. I'll create another issue for that.
|
OK I've made the noWrap property one-way bound now instead of two-way. I'd also like some feedback on my unit test. I'm new to unit testing angular, and I'm not sure that I'm binding properties to the directive controller's scope properly, i.e. via the compiled element |
| @@ -73,6 +73,42 @@ describe('carousel', function() { | |||
| expect(indicators.length).toBe(3); | |||
| }); | |||
|
|
|||
| it('should stop cycling slides when noWrap is truthy', function () { | |||
| scope.noWrap = function () { | |||
There was a problem hiding this comment.
should be scope.noWrap = true;
|
Updated based on feedback from @chrisirhc |
| @@ -75,6 +74,11 @@ angular.module('ui.bootstrap.carousel', []) | |||
| $scope.next = function() { | |||
| var newIndex = (self.getCurrentIndex() + 1) % slides.length; | |||
|
|
|||
| if ((newIndex === 0) && $scope.noWrap()){ | |||
There was a problem hiding this comment.
The parentheses around newIndex === 0 isn't needed.
|
This PR looks good to me. The above is just a style comment that isn't important. |
|
Looking at this PR and thinking about it some more, this should also support preventing going from a slide of index 0 to the last slide - can you update this PR with that change too? This also needs documentation updates as well. |
|
Sorry for the late reply. I can definitely update this PR with that change. Should I handle the documentation updates as well? Also, should I pull/rebase on top of master before making these changes? |
|
@ChewTeaYeah Rebase & docs yes please! |
|
I've made the changes and modified my unit tests as well, but I'm not sure where I should make the docs change. Carousel has a short blurb about its functionality, but does not explicitly specify its parameters. Should I add to that blurb? |
|
@wesleycho can you provide some guidance on the documentation updates per my last comment? This is ready to go other than documentation and I'd love to see it merged in soon :-) |
|
Yes, adding the blurb for the carousel element would be good. |
|
@wesleycho I've updated the docs and added an option to the demo to enable noWrap on the carousel's slides. I also rebased and fixed conflicts, so this is ready to go! |
|
There you go :) - it will make it out in the next release. |
|
Thanks! This was my first contribution to anything open source, ever. Hope its the first of many! |
|
Keep at it, there's lots to learn and much to do :) |
Pausing the carousel timer function after the last slide is rendered if the noWrap option on the scope is present and set to a truthy value. Addresses #3397