-
Notifications
You must be signed in to change notification settings - Fork 6.7k
fix(carousel): using setInterval to get rid of dangling $timeouts #1423
Conversation
Signed-off-by: Josh Kurz <josh.kurz@turner.com>
@@ -57,8 +57,6 @@ angular.module('ui.bootstrap.carousel', ['ui.bootstrap.transition']) | |||
} | |||
self.currentSlide = nextSlide; | |||
currentIndex = nextIndex; | |||
//every time you change slides, reset the timer | |||
restartTimer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshkurz Removing this one I think has a side effect on the time between transitions of the slides. What I mean is that if for example the interval is 5 sec and at 3 sec you move to the next slide, then that one will stay only for 2 sec instead of 5. There are no tests for this, but I think this is the expected behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'll get that back in there then. Good catch.
Thanks
Josh Kurz (mobile)
Signed-off-by: Josh Kurz <josh.kurz@turner.com>
I didn't really like the idea of resetting the timer every time we call select. Sometimes the interval is actually on track and we dont need a new one. We only need a new interval if an un-pause event occurs, or the user clicks the next or prev button and interrupts the current interval. So I added a clicked flag in the template and passed it down to the select function, so that goNext can tell if it needs to reset the interval. There are a couple options for tests.
Wondering what way you guys think is best in terms of testing? |
I'm wary of using |
} else { | ||
$scope.pause(); | ||
} | ||
$timeout(angular.noop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be $scope.$apply()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
causes digest errors. This is the safe way to run a digest without checking for the $$phase on the scope.
@joshkurz Sorry for the late response, but I am also not keen of replacing the Also, I am not sure about the distinction you make between the transition made by clicking and the one made programmatically. I don't think there is a use case that someone will expect the timer not to be restarted, no mater how the transition was caused. |
If I understand correctly this got fixed by #1451 |
so for the Angular 1.2 branch are we planning to use $interval? On Mon, Dec 23, 2013 at 9:09 AM, Pawel Kozlowski
Josh Kurz |
There are a couple issues with how the carousel currently handles its interval functionality.
This PR uses setInterval instead of $timeout.
If we switched to using $interval in the 1.2.x branch, everything would still work the same. The syntax would be a little cleaner and the tests would not have a need for the jasmine.clock object.