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

Configurable scroll container #51

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

Conversation

josdejong
Copy link

See #50

Copy link

@cvlab cvlab left a comment

Choose a reason for hiding this comment

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

Singleton is not good now, I'l add also something like this:

const jump = function () {
	return jumper().apply(undefined, arguments);
};

export default jump

@Sam-Hoult
Copy link

Sam-Hoult commented Apr 9, 2017

Using this version of Jump going forward until container support is merged into main.
Came across error when at the top of the page as location() return operator would count 0 as falsy and accept undefined, causing NaN errors. Changed to:

  function location() {
    let top = container.scrollTop || container.scrollY || container.pageYOffset;
    top = typeof top === 'undefined' ? 0 : top;
    return top;
  }

@MilllerTime
Copy link

I would love to see this merged.

One potential block is that master was updated to use ESLint a few days after this PR was created. Once merged, ESLint is complaining, but the fixes are simple:

  • Add a space after all function names
  • Add a space after switch keywords

@scottyeck
Copy link

@callmecavs Are there any plans to merge this and support this feature? Looks like there's quite a bit of interest.

@dehypnosis
Copy link

dehypnosis commented Feb 23, 2018

Any reasons to hesitating about merging this PR?
currently using forked version....

break

case 'string':
container = document.querySelector(options.container)
Copy link

@mauskin mauskin May 2, 2019

Choose a reason for hiding this comment

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

Wouldn’t it be better to look up the container going from the target up instead of from the document down?

container = target.closest(options.container)

It would exclude errors where the container selector would be a common class name instead of id. Also might be faster.

@jprodrigues70
Copy link

any updates?

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.

None yet

8 participants