Pagescroll bug fix #30
Conversation
|
Hello, Thanks for the pr. I notice it doesn't pass the unit tests, can you take a look at that? |
|
Hello, it seems test fails because it throws resize event with no width actually changing, so after my fix it doesnt triggers resize function. I don't know at this moment is this expected behaviour or not, but IMHO you shouldnt trigger resize when it was no really resizing. 'window.dispatchEvent(new Event('resize'));' <- thats why test fails. |
|
I don't remember exactly, but I'm pretty sure it's their for a reason. I think it will automatically be resolved if you fix the review comment I gave because than there's something to |
|
Hm i m sorry, but i cant find where did u post review comments. And here i can see 'no reviews' in reviewers list. |
|
I posted it over here: |
|
I couldnt see your comments because it seems you didnt finish your review. By the way, i ll think, how to do fixes you wrote about. |
| @@ -474,7 +475,15 @@ angular.module('hl.sticky', []) | |||
| } | |||
|
|
|||
| // bind events | |||
| throttledResize = throttle(resize, $stickyElement.defaults.checkDelay, {leading: false}); | |||
| throttledResize = function(event) { | |||
There was a problem hiding this comment.
The throttledResize method isn't throttled anymore. The throttling is also recreated everytime the window resizes, so that can't be good. I think that needs to be reverted so it makes sense again by wrapping the resize method inside another method with your changes.
There was a problem hiding this comment.
It's already triggering all the time on scroll. Resize events are significantly more rare, so what's the purpose of even throttling?
To prevent too many timers, can just put:
$timeout.cancel(timeout);
right before the $timeout and set delay to like 10-20ms, and add a call directly to resize() inside the init method too.
|
|
||
| var newWidth = window.innerWidth; | ||
|
|
||
| if (windowWidth != newWidth) { |
There was a problem hiding this comment.
Actually only checking this will cause a bug. If someone changes the height, and the element was near the start/end of where it was sticky, that could change and this would cause it to not be updated
When using angular sticky directive in project, found the bug with page scrolling at new element on page adding. It was because throttledResize function triggered even if window not actually resized. This pull request fixes that bug with comparing width before and after resize event.