Skip to content

Conversation

Ahrengot
Copy link

Hey Florian,

As the title suggests, I added a few extra things to your otherwise great jQuery plugin.

  1. AMD Ready following this method http://stackoverflow.com/a/11890239/641755
  2. Add package.json so this can be submitted to jamjs.org etc.
  3. Make it work with both <ul> and <ol> elements.
  4. Add example folder with a basic example in it.
  5. Optimize and modernize the code slightly.

@polarblau
Copy link
Owner

Many thanks! Looks great.
I'm not sure if the example should be included in the code base — is this considered a standard approach in a relevant environment? Did you take a look at the gh-pages branch? Maybe we could update the example there if necessary?

@Ahrengot
Copy link
Author

Ahrengot commented Aug 1, 2013

Hey again,

I actually removed that ".sticky" part because I didn't know what it did. Looking through the jquery.com docs didn't help me much, so I figured it was something to do with the old .bind() method and removed it to be sure things wouldn't break after switching to .on().

However, I just looked through the jQuery source and it turns out it's namespacing for events (If I'm correct), which is pretty cool. Thanks for the tip!

I pushed another commit and it looks like that attached itself to the pull request automatically :octocat:

@Ahrengot
Copy link
Author

Ahrengot commented Aug 1, 2013

Oh, and regarding the example:

I made it so that the only thing that is pulled down by package managers is src/jquery.stickysectionheaders.js. That's how Backbone does it, so I figured it was a nice way to go. The package.json file liks to the original github repo anyway so people get get alle the extras there.

I think for people going to GitHub and clicking the "download zip" button, it's nice to have an example you can open up and copy/paste the code from.

@polarblau
Copy link
Owner

Yep, namespacing events is really handy! And thanks for explaining your thoughts on the example — let's keep it in! It seems that the tests are currently broken #9 so I can’t verify that everything works, but that has nothing to do with your changes. I’ll try to fix them and then merge the PR.

Would you mind documenting the AMD support in the README? We might also be able remove the examples from there as we now have the example directory.

Many thanks again!

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