Feature request: offset

[elado]

Add the option to add an offset to the scroll. Specifically solves a scenario where there's a fixed header and scrolling to an element scrolls beyond the header.

[isellsoap]

Such an option would be really useful!

[cideM]

Seems straightforward, I'll submit a PR towards the end of this week and if it doesn't suck completely, it can maybe be of some use to you guys. (would be my first PR, hence the "disclaimer")

update
I actually wrote a first version that now also accepts options as either a list of params or an options object, but I'll wait with the PR until I've had time to ponder my code a little. Will upload tonight though, then you guys can try it out @isellsoap @elado

update 2
#32

[sbernheim4]

Has this PR been made/given up on. Cause this would be super helpful. I think all it would take is an extra parameter for the function on line 30, and then to add to the end of line 33 - offset if offset is the name of the parameter. If no one already has could I try and submit a PR for this?

[cideM]

As outlined in my comment I submitted a PR. Recently someone else submitted another PR with that option. It's up to the repository owner to decide what is going to happen with either of them.

[superhussain]

@cideM I've been added as a collaborator and want to add this feature but am torn about which implementation to go with. Both your pull request and my own aren't the most friendly for backwards compatibility and I want to come up with a solution that would fill everyone's needs.

I love your idea of adding an optional arguments object, and definitely would go with that implementation if we didn't have to think about backwards compatibility. We could add that as part of a next major release if you're down for that.

What do you think? @cideM @elado @alicelieutier

[ahoym]

Giving an outside opinion here, but I think in terms of longer term maintainability, we should really move towards object based options instead of tacking on more and more place specific arguments. Having an options object will allow us to be more flexible with future properties.

Here's a potential solution:

You could pin/tag the current master to v1.0.0 and npm publish it. Then, implement the object based options and when that's merged, pin that to v2.0.0. The reason for the major version bump is because scrapping position based arguments entirely would be completely changing the API of smoothScroll.

At that point, whoever needs the old implementation will be using 0.x.x or 1.x.x. Anyone who wants to use the new one will use 2.x.x. This would also be a good time to implement a CHANGELOG.md documenting the difference between the two.

If you wanted to be extremely thorough, you could add a migration guide that would have a simple example:

"Switch this:"

smoothScroll(myEl, 1000, myCallback, myContext);

to this:

smoothScroll({
  el: myEl,
  duration: 1000,
  callback: myCallback,
  context: myContext
});

Again, just my two cents

[cideM]

Generally I have no objections, as it's my first PR, I didn't even think
this would get added. :) But what's preventing backwards compatibility? I
only added the optional offset, the old arguments list still works. My PR
as far as I tested understands whether it's called with an object or
individual arguments.

On Fri, Oct 21, 2016, 16:26 Hussain Abbas [email protected] wrote:

@cideM https://github.com/cideM I've been added as a collaborator and
want to add this feature but am torn about which implementation to go with.
Both your pull request and my own aren't the most friendly for backwards
compatibility and I want to come up with a solution that would fill
everyone's needs.

I love your idea of adding an optional arguments object, and definitely
would go with that implementation if we didn't have to think about
backwards compatibility. We could add that as part of a next major release
if you're down for that.

What do you think? @cideM https://github.com/cideM @elado
https://github.com/elado @alicelieutier
https://github.com/alicelieutier

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#31 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEDNiXe0oSfzPwRMNTkiXUytnwi9D23uks5q2MuggaJpZM4JnEm2
.

[connor-baer]

I just found this lovely library and I also need an option to set the offset for my use case. What's the status of this issue/the PRs?

[alicelieutier]

I don't think offset should be part of smoothscroll. I understand the problem it solves: everyone has headers they want to work around.

Offset is not a problem of scrolling, it is a construction problem of the site.

Consider this scenario:

We add offset it to smoothscroll, and you scroll to my.site.com#linkA using smoothscroll.
It shows your anchor correctly (not hiding behind the header)

Now you share that specific link to someone and they click it: the page with load and the anchor will now be hiding behind the header.
That's because the anchor being at the top of the page is the correct HTML position for that anchor. I don't want to make smoothscroll change the standard behaviour and hide bugs to the developer.

--
It is an annoying problem, but there are ways to solve this that would work even when the link is opened directly, and I think these would be the right solution, not changing the scrolling itself.

https://pixelflips.com/blog/anchor-links-with-a-fixed-header
https://stackoverflow.com/questions/4086107/html-positionfixed-page-header-and-in-page-anchors

--
I may be wrong, too, please argue if you do not agree :)

[superhussain]

I disagree. It's unfair to point the blame at the developer of the site for not constructing it the "right way". SmoothScroll is used by a lot of people and organizations I'm sure, it's sometimes not practical to change the way the site is constructed to have the ability to scroll between links. Having the flexibility within the library would allow people to use it how they like, without having to change their existing setup. The libraries where I can pass my own configuration into, then have everything just work, those are my favourite.

The suggestion @ahoym made with the separate versioning would be the best approach I think. That would satisfy the bulk of the users without killing compatibility for existing users.

[alicelieutier]

Thanks @superhussain !
You're right, it's unfair to put the blame on the developer and I apologize for having expressed myself harshly.
It is a hard and recurring problem, and I'm not suggesting it is wrong to need that feature.

What I wanted to point out, though, is that doing this will not correct the link placement in all cases, but only on scrolling, which feels unsatisfactory to me. The placement will still be wrong on pressing back button, opening links with hashes directly, etc.

You say "The libraries where I can pass my own configuration into, then have everything just work, those are my favourite." but adding offset is not going to have everything just work, sadly, it's not a complete solution.
If you feel strongly that it should still be implemented as a partial solution, then why not.

[alicelieutier]

Also, quick CSS solution for headers problems:
If you have this html:

<!DOCTYPE html>
<html>
<head>
	<title>test</title>
</head>
<body>
	<div id="header"></div>
	<div id="content"></div>
</body>
</html>

And your header is fixed, then your CSS should look like this to avoid header issues:

#header {
    position: fixed;
    top: 0;
    height: <HEADER_HEIGHT>;
}

#content {
    position: fixed;
    top: <HEADER_HEIGHT>;
    overflow: scroll;
    height: 100%;
}

Then content never hides behind your header (you can test this with a transparent header), and both scrolling and navigating directly to hash links will work.

[elondaits]

I'm currently trying to use the library to put an element into view. It's not an anchor (it's highlighted text) and there's no associated internal URL. I would like the ability to configure an offset and it has nothing to do with headers... rather to reproduce "scrollIntoView" with greater browser support.

[alicelieutier]

Hi @elondaits, I'm not sure I understand your comment.
You can scroll to any element using smoothscroll, not just anchors.
When you talk about scroll into view, do you mean to say you want the scroll to end when the element is on the screen, whether it's at the top or the bottom of the screen, or is it something else?

[elondaits]

My comment is in the context of this feature request and the reasons you gave not to implement it.
You mentioned it's wrong to give an offset when scrolling to an anchor because people navigating to that anchor when using a link would have the anchor covered by the sticky header.

My point is that in my use case my need for the offset option is not related to sticky headers. I'm scrolling to a highlighted text fragment and I want to scroll to a position relative to it (so the fragment appears in the middle of the screen)... so it would have been nice to have the offset option.

Lastly, there's a standard scrollIntoView method that allows you to scroll so an element "comes into view". By default Chrome scrolls so the element in the middle of the viewport. This method, however, does not have smooth scroll in many browsers (IE, Safari, etc.). I was actually testing your library to replace scrollIntoView for something with better cross browser compatibility... but without the offset option I had to resort to something else.

[marcaaron]

@elondaits I actually also came here looking for an alternative to Element.scrollIntoView() / caniuse. smoothScroll has me covered in the "smooth" department, but I kind of love how easy it is to center an element with scrollIntoView()!

My solution was to make some edits to the getTop function so the element winds up centered vs. at the top of the window. What do you think? Would this create any issues?

var getTop = function(element, start, context) {
    if(element.nodeName === 'HTML') return - start

    const el = element.getBoundingClientRect();
    let ctxHeight;

    if(typeof context.getBoundingClientRect === 'function'){
	ctxHeight = context.getBoundingClientRect().height;
    }else{
	ctxHeight = context.innerHeight;
    }
 return el.top + el.height - (ctxHeight/2) + start;
}

in this example you'd also need to pass context to the var end = getTop(el, start, context) so it can determine the center of your scrollable area.

[elondaits]

I didn't consider the option of modifying the code because then if I need to update the library to a new version to fix some bug I need to remember to add my fix in again. It's much better if the option is implemented in the official repository.