Skip to content
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.

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

Update scrollLeft in response to 'scroll' event #172

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nilclass
Copy link

Fixes #171

Previously the glider dots were not updated when scrolling. Also when dragging after scrolling, the scroll offset would jump back to where it was before the scrolling as soon as the drag started.

I believe the bug was introduced by the changes in #146. Oddly those changes don't seem to be part of the minified file on current master (search for scrollLeft - it only shows up as .ele.scrollLeft), which is why the problem does not appear in the demos.

Previously the glider dots were not updated when scrolling. Also when
dragging after scrolling, the scroll offset would jump back to where
it was before the scrolling as soon as the drag started.
@@ -83,7 +83,10 @@
// set events
_.resize = _.init.bind(_, true)
_.event(_.ele, 'add', {
scroll: _.updateControls.bind(_)
scroll: function (event) {
_.scrollLeft = _.ele.scrollLeft
Copy link
Author

Choose a reason for hiding this comment

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

The only real change is this line. The hunks below were changed by the linter.

@NickPiscitelli
Copy link
Owner

Nice catch. This looks good and makes sense to me, but I'm wondering if the assignment should just be placed in updateControls instead. While the naming convention isn't the most semantic since the function appears to do more than actually update the controls, it's already setting a few other core values such as slide and page.

@nilclass
Copy link
Author

nilclass commented Mar 9, 2021

Thanks for taking a look at the changes!

but I'm wondering if the assignment should just be placed in updateControls instead.

To be honest, I haven't fully grasped why both _.scrollLeft and _.ele.scrollLeft are needed. So I can't really judge if it breaks anything to set _.scrollLeft in updateControls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dots not always working when scrolling (differs between .js and .min.js)
2 participants