Skip to content

Add offset feature to scrollBehavior #1501

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

Merged
merged 1 commit into from
Jun 16, 2017
Merged

Add offset feature to scrollBehavior #1501

merged 1 commit into from
Jun 16, 2017

Conversation

sigerello
Copy link
Contributor

scrollBehavior function can return a scroll position object in the form of:
{ selector: string }, but there is no way to add some offset to element specified by this selector.

For example, if I have navigation bar with height of 100px, fixed on top of the page, it's not enough to specify just element's selector. window will be scrolled right to that element and navigation bar will overlay it.

I've added an ability to specify an offset of element to address such issue.
Now scrollBehavior can return position in form of:
{ selector: string, offset: { x: number, y: number } }.

@posva
Copy link
Member

posva commented Jun 10, 2017

Thanks a lot for the PR. However, this is something that can already be achieved by getting the offset yourself and returning the position directly. Even better: you can compute the margin too so you don't write a fixed value
You have the scroll behavior logic at https://github.com/vuejs/vue-router/blob/dev/src/util/scroll.js#L79

I think this kind of helper could be provided in a separate package although it probably exists already
I still want to ask for other's opinions but it's better if we don't add multiple ways of doing the same thing

@sigerello
Copy link
Contributor Author

@posva you are right.
In my case I can go this way:

const getElementPositionWithOffset = function(selector, offset) {
  const el = document.querySelector(selector);
  const docEl = document.documentElement;
  const docRect = docEl.getBoundingClientRect();
  const elRect = el.getBoundingClientRect();
  offset = typeof offset === "object" ? offset : {};
  offset.x = typeof offset.x === "number" ? offset.x : 0;
  offset.y = typeof offset.y === "number" ? offset.y : 0;
  return {
    x: elRect.left - docRect.left - offset.x,
    y: elRect.top - docRect.top - offset.y
  };
};

export default new VueRouter({
  mode: "history",
  scrollBehavior: function(to, from, savedPosition) {
    if (to.hash) {
      return getElementPositionWithOffset(to.hash, { y: 100 });
    } else if (savedPosition) {
      return savedPosition;
    } else {
      return { x: 0, y: 0 };
    }
  },
  routes: [
    ...
  ]
});

This custom function getElementPositionWithOffset nearly the same as you pointed me out to, with little additions.
Thanks for your answer!

@sigerello
Copy link
Contributor Author

sigerello commented Jun 10, 2017

@posva You say you don't wan't to add multiple ways of doing the same thing and it's 100% right.

In this regard let me propose some improvements to API.

  1. Deprecate the ability to specify { selector: string } and remove it in future versions. And of course add some examples such as mine in docs, so people can understand how to deal in such specific cases. API will be a little cleaner.

  2. Rename scrollBehavior function to scrollPosition because really there is no behavior, but just scroll position detection/configuration. Real behavior is window.scrollTo(...).

  3. Add scrollHandler function as an option and use it this way inside handleScroll function:

let position = getScrollPosition();
...
let scrollHandler = router.options.scrollHandler
if (!scrollHandler) {
  scrollHandler = function(to, from, position) {
    window.scrollTo(position.x, position.y);
  }
}
scrollHandler(to, from, position);

Using this option you can redefine default scroll, add some scroll animation etc.

The main point of this is to simplify and separate logic of:

  • scroll position setting
  • scrolling

What do you think?
If you approve, I can try to make a new PR.

@yyx990803
Copy link
Member

I actually find this feature pretty handy and it would be tedious for the user to manually calculate this.

@sigerello FYI if we can, we try our best to avoid breaking changes. IMO renaming doesn't serve too much benefits, and you can already use scrollBehavior to perform custom scrolling (and just return nothing).

@yyx990803 yyx990803 merged commit 023c899 into vuejs:dev Jun 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants