Skip to content

Removed JQuery dependency #380

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

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

Removed JQuery dependency #380

wants to merge 46 commits into from

Conversation

cyruscook
Copy link

I wanted to use this on a website I'm building that doesn't have JQuery, so I rewrote the plugin to use vanilla JS.

It's seems to be working fine, and I'm implementing it into my own project right now, however I wondered if you'd like to include my changes in the base repo? There's practically no reason to use JQuery as it takes 100ms to load (on my computer) and nearly everything is achievable without it.

If you do want to use my code, please tell me as I will have to make some changes.

  • First off, I use tabs instead of spaces, but I can revert that.
  • Secondly, I put my { on a new line, however I can revert that too.
  • Thirdly, I couldn't get the animations to be the same when animating it with vanilla JS. I'm not too sure why this is, however there will be a difference both in speed and interpolation when using the new code (however I've gotta say I prefer this version's).
  • Fourth, I'm not using NodeJS so I haven't debugged it for that, however I suspect it won't be working for that.

@lukehaas
Copy link
Owner

lukehaas commented May 3, 2019

This is an amazing contribution, thank you.
I will review your PR and try it out on the example pages. If it's all good, I'll certainly want to merge this in.

@lukehaas
Copy link
Owner

lukehaas commented May 3, 2019

One thing to note, as you're using modern JavaScript, we'll need to introduce Babel to transpile this for older browser support.

@cyruscook
Copy link
Author

cyruscook commented May 3, 2019

Ah, ok, hadn't been thinking about that. Would that involve a development file and a production file? I guess that would also mean that we could compress the production file which would be beneficial.

I've given you access to my fork btw if that helps (https://github.com/cyruscook/Scrollify/invitations)

@cyruscook
Copy link
Author

cyruscook commented May 3, 2019

I believe line 917 (if(panel.originalEvent)) is checking if panel is a JQuery event obj? I'm not too sure why that is however, as if I'm not mistaken panel is meant to be either the index or name of a panel. Do you know what that's meant to be for?

@lukehaas
Copy link
Owner

lukehaas commented May 3, 2019

Regarding (if(panel.originalEvent)) I think the intention here was to enable something like this:
$('a').click($.scrollify.move)

But I don' think I ever documented this so it's probably not used like that by anyone.
Happy for you to remove this.

@cyruscook
Copy link
Author

cyruscook commented May 3, 2019

I've compressed the file and run it through babel. I've fixed a lot of things, but I've added a Known Issues section to the readme. I'm currently in the process of debugging them.

@cyruscook
Copy link
Author

I'm having some trouble currently, could you help me?

When offset is called on page load, it returns incorrect values, which is why the page snaps. If you check the array heights, its been populated with these incorrect heights. For some reason though, it seems that running the function again makes it work - I'm guessing if this is a problem with the page not fully loading yet?

@lukehaas
Copy link
Owner

lukehaas commented May 6, 2019

@cyruscook I'll take a look into that

Copy link

@geekymemo geekymemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi,
.height() is jquery method for js must be .offsetHeight
if (Math.floor(elements[index].offsetHeight / portHeight) > interstitialIndex) {

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.

3 participants