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

VueRouter - Allow scrolling other than window #38

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

Conversation

phouri
Copy link

@phouri phouri commented May 23, 2019

@pimlie
Copy link

pimlie commented Jun 26, 2019

Would using scrollIntoView be an option as a way to solve the first drawback about old browser quirks?

@phouri
Copy link
Author

phouri commented Jun 26, 2019

ScrollIntoView does something else afaik - it scrolls the parent of a child to get the child into view, and does not scroll the element itself.

@pimlie
Copy link

pimlie commented Jun 26, 2019

This should work:

if (isObject && typeof shouldScroll.selector === 'string') {
    const el = document.querySelector(shouldScroll.selector)
    if (el) {
      el.scrollIntoView()
    }

we'd only need to add additional logic for isPop

@phouri
Copy link
Author

phouri commented Jun 26, 2019

It's not just scrolling into view - it's keeping the last position, scrolling element can be huge - thousands of pixels, scrolling to the top of it (or center or any other option) - is not the required behavior, it should be where it was before navigation.

@pimlie
Copy link

pimlie commented Jun 26, 2019

Hmm, yes. Isnt that actually managed by saveScrollPosition anyway? The position calculated in scrollToPosition doesnt seem to be used for that.

@phouri
Copy link
Author

phouri commented Jun 26, 2019

save scroll position saves it only from the window, which is in some cases not the scroller.

Check out my draft PR for a full example - it saves the new position and uses it on the new element.

@posva posva added the router label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants