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

Added optional offset paramter #36

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

superhussain
Copy link
Collaborator

@superhussain superhussain commented Oct 10, 2016

I know there was another pull request with this option but I saw that there were conflicts so I made my own (without the arguments object). Shoutout to @cideM for beating me to it!

Example usage:

var smoothScroll = require('smoothscroll');

var exampleBtn = document.querySelector('.example-button');
var exampleDestination = document.querySelector('.example-destination');

var handleClick = function(event) {
  event.preventDefault();

  // This will scroll 100px above the exampleDestination element in 300 milliseconds
  smoothScroll(exampleDestination, -100, 300);
};

exampleBtn.addEventListener('click', handleClick);

@alicelieutier
Copy link
Owner

Thanks for you PR.

I like your diff, but it is making 2 changes that make this not retro-compatible:

  • changing the order of the arguments
  • scoping on .scroll

Changing the order of the arguments is just confusing, I'd like to avoid it.

Now for the scoping, some background:
I do think the scoping is something that needs to be done at some point.. there sure seems to be people asking for it.
The main reason why it never got in is that it was not the point of the original library. The initial idea was basically to add the library to a page and let it do its magic on the links.. the API came as an afterthought. Now this was several years ago, when front-end was not exactly what it is now. You would write a html page and then add libraries in between <script> tags.
Anyway, the web has changed, the world has changed, but I am not building websites at the moment and I don't feel that I am the best person anymore to choose the best approach on this.

So I guess this change should be done.
Please check the documentation, because I think there are more things to change there relative to scoping.

If you do use this library and think you have some time to take it to a place where it is more adapted to today's web, then I'd be happy to add you as an owner.

@superhussain
Copy link
Collaborator Author

Hey, thanks for the response!

I'll be completely honest, I mainly made the changes for myself to use in my own project. The changes I made were for based on how I would make a library like this, whatever made the most sense in my mind.

I rearranged the arguments based on usage of the arguments. I found myself using offset a lot more than the animation duration so I put it before. Scoping to a class because I was using React and React doesn't behave well with hash links -- without specifically adding event.preventDefault() to all hash links. Instead I added event.preventDefault() to all links with the scroll class.

Of course this wouldn't be the same use-case for everyone so it makes sense not to pull my changes. I'll spend some time tweaking things to be more universal especially with the scoping. Not sure if scoping just to a class called scroll would be ideal for every project haha.

I do use this library. I do think I have some time to take it to a place that's more adapted to today's web. I'd be happy to be added as an owner! 😁

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.

2 participants