fix(carousel): replaced $timeout with $interval when it was wrong #2776
Conversation
|
This also appears to address #1308. It would be really nice to get this in since apps whose login page have a carousel cannot be tested with Protractor without major hacking without it. We hit this today after upgrading ui-bootstrap to 0.11.2. |
|
I can verify that this fixes the problem with Protractor when there is a Carousel on the page. It also passed all of the Karma unit tests. |
| } | ||
| } | ||
|
|
||
| function timerFn() { | ||
| if (isPlaying) { | ||
| var interval = +$scope.interval; | ||
| if (isPlaying && !isNaN(interval) && interval>=0) { |
There was a problem hiding this comment.
Is there any reason this if statement was moved from the restartTimer above?
This was meant to check that the interval created makes sense, it seems unsafe to remove it from line 109. An additional check here may make sense, but I think it should also still be left intact above.
There was a problem hiding this comment.
I was experimenting with removing this check earlier, I think removing it should be okay. Did you experience a bug after removing this?
|
Thanks @Gahen . This looks pretty good. Just reviewed it. Should be okay to merge with those changes in place. |
Gahen
force-pushed
the
master
branch
2 times, most recently
from
632eae4
to
7db93a8
Compare
|
Hey! Thanks @chrisirhc for the comments, I made the appropriate changes, your points seemed valid to me. I already updated my repo with those changes. |
|
👍 |
| var $rootScope, $compile, $controller, $timeout; | ||
| beforeEach(inject(function(_$rootScope_, _$compile_, _$controller_, _$timeout_) { | ||
| var $rootScope, $compile, $controller, $timeout, $interval; | ||
| beforeEach(inject(function(_$rootScope_, _$compile_, _$controller_, _$timeout_, _$interval_) { |
There was a problem hiding this comment.
_$timeout_ can be removed.
|
One more thing I missed earlier. Great work! |
|
Nice catch, I just removed it and pushed it |
| @@ -152,12 +152,12 @@ describe('carousel', function() { | |||
| //no timeout to flush, interval watch doesn't make a new one when interval is invalid | |||
| testSlideActive(0); | |||
| scope.$apply('interval = 1000'); | |||
| $timeout.flush(); | |||
| $interval.flush(scope.interval); | |||
| testSlideActive(1); | |||
| scope.$apply('interval = false'); | |||
There was a problem hiding this comment.
This test now needs $interval.flush(5000); here to actually test these values.
I found that this actually causes the test to fail on $interval function because it's calling $interval with false as the timeout.
There was a problem hiding this comment.
Shouldn't it be $interval.flush(1000)?
Looking at it more closely I don't get why here we use $apply here to set scope.interval value and it's directly modified on the last test as scope.interval = 2000;. Any clue?
|
Thanks for the quick edits! Looked through again. It looks like an interval value of false or 0 will cause the tests to crash when flushing the $interval. Looks like we'll have to update the documentation so that we indicate that we're not allowing 0 to be the interval. I don't think that was valid before anyway and I doubt anyone sets it to 0. |
Gahen
force-pushed
the
master
branch
2 times, most recently
from
683e488
to
0957781
Compare
I had a to tweak a bit two tests since $interval never throws an exception after flushing, hope I did it OK. Ammend: addresses @chrisirhc critics
|
Thank you! Looks good to me. Merging this soon. |
|
Thank you! |
I had a to tweak a bit two tests since $interval never throws an exception after flushing, hope I did it OK. Ammend: addresses @chrisirhc critics Fixes angular-ui#1308 Fixes angular-ui#2454 Closes angular-ui#2776
|
Any chance this can be merged for your next release? This issue is blocking me from using angular-ui as I'm using protractor. Thanks. |
I had a to tweak a bit two tests since $interval never throws an exception after flushing, hope I did it OK.
This addresses #2454