Skip to content

Configurable scroll container - #51

Open
josdejong wants to merge 1 commit into
callmecavs:masterfrom
josdejong:configurable_container
Open

Configurable scroll container#51
josdejong wants to merge 1 commit into
callmecavs:masterfrom
josdejong:configurable_container

Conversation

@josdejong

Copy link
Copy Markdown

See #50

@cvlab cvlab left a comment

Copy link
Copy Markdown

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

Sam-Hoult commented Apr 9, 2017

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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

@dehypnosis

dehypnosis commented Feb 23, 2018

Copy link
Copy Markdown

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

Comment thread src/jump.js
break

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

@mauskin mauskin May 2, 2019

Copy link
Copy Markdown

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
Copy Markdown

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.

8 participants