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

Vanilla JS port #41

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

Vanilla JS port #41

wants to merge 3 commits into from

Conversation

martgnz
Copy link

@martgnz martgnz commented May 21, 2020

This is a big PR. Sorry about that!

This is a proof of concept of a scrollStory version that doesn't rely on jQuery. I had to rewrite some parts but I've tried as much as I can to maintain the same structure and methods of the jQuery version (and added some functions to cover things that are uneasy with vanilla JavaScript).

I've tested it and so far everything seems to work fine. The events run, binding with data seems to work fine and adding tags and categories too. If you think this is worthwhile additional testing and cleansing will be needed, as I didn't remove some conditionals that can still accept a jQuery object, etc.

Caveats

First, an easy one: the animations and easing controls for the keyboard option don't work. There's a smooth option in the scrollTo method which I've used and could be enabled or disabled with an option. Maybe there's an easy way to replicate the jQuery animations without depending on another library but I didn't get so far.

My main problem is that I've been unable so far to assign the main instance to a variable and trigger events with it. This seems like a blocker to me, but I am not sure how to do that without changing the structure of the entire file. With jQuery this seems to be easier as you seem to be running an anonymous, self-executing function and then saving the main ScrollStory instance in a jQuery object that can be surfaced to the user. Now I am exposing a normal function at the top that calls the ScrollStory method with the options, and I can't find a way to add those in and run ScrollStory if the main function is self-executing.

var story = scrollStory({
  container: '#container'
});

// story is undefined :(
story.on('itemblur', (ev, item) => {
  console.log('blur')
})

Maybe there's a way to fix this without changing the layout of the file so much. I would be grateful if could point me towards a solution!

TLDR

I would like to know if you're interested in this change. My main motivation is that other libraries like Scrollama work mostly fine, but at the end I have faced some problems with scroll events that are hard to debug (specially on mobile). scrollStory doesn't seem to have those. The problem is including jQuery, a big trade-off for some people if you're not using it already.

In the future this could also lay the ground to a modernisation of the library itself, implementing a build system like rollup and a rewrite using classes and ES2015.

@sjwilliams
Copy link
Owner

Whoa, Martín! This has been on my todo list for... years. Thanks for giving this some thought and a first pass.

In short, yes, I'm interested in what you've done. I'm away from work a few days and deep in some covid tasks when I return next week.

If you're not in a hurry, I'm happy to take a thorough look soon. In addition to dropping jQuery, I also want to modernize the code, much as you suggest, and clean up some mistakes in the API that have irked me for years.

@martgnz
Copy link
Author

martgnz commented May 22, 2020

Nice, this is great news! I was afraid of you not wanting to introduce such a breaking change (which would be understandable).

I'm in no rush, please take your time and let me know if you need any changes or edits, I'm happy to do them. Would be great to see the library modernised in the future as well 👍

@martgnz
Copy link
Author

martgnz commented Aug 14, 2020

@sjwilliams did you have time to check? Maybe it is better to work on a branch and not make every change at once (bundler, es6, etc). I know it's a big PR!

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