Skip to content

Pagescroll bug fix #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pavel1992 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Pagescroll bug fix #30

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ada9169
fixes bug with unneccessary throttledResize call when window width no…
Aug 9, 2017
bad027e
wrong var name fix
Aug 9, 2017
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
13 changes: 11 additions & 2 deletions js/angular-sticky.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,7 @@ angular.module('hl.sticky', [])
$get: function($rootScope, $window, $document, $log, DefaultStickyStackName, hlStickyElement, hlStickyStack, throttle) {

var windowEl = angular.element($window);
var windowWidth = window.innerWidth;

var unbindViewContentLoaded;
var unbindIncludeContentLoaded;
Expand All @@ -474,7 +475,15 @@ angular.module('hl.sticky', [])
}

// bind events
throttledResize = throttle(resize, $stickyElement.defaults.checkDelay, {leading: false});
throttledResize = function(event) {
Copy link
Owner

@harm-less harm-less Aug 9, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@phazei phazei May 17, 2018

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor

@phazei phazei May 17, 2018

Choose a reason for hiding this comment

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

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

throttle(resize, $stickyElement.defaults.checkDelay, {leading: false})(arguments);
windowWidth = newWidth;
}
};
windowEl.on('resize', throttledResize);
windowEl.on('scroll', drawEvent);

Expand Down Expand Up @@ -687,4 +696,4 @@ angular.module('hl.sticky', [])
}
};
};
});
});