stickySectionHeaders icon indicating copy to clipboard operation
stickySectionHeaders copied to clipboard

Published 20 hours ago • verified publisher icon polarblau

AMD Ready, Package.json and minor optimization

Open Ahrengot opened this issue 12 years ago • 4 comments

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.

Ahrengot avatar Jul 31 '13 11:07 Ahrengot

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?

polarblau avatar Jul 31 '13 18:07 polarblau

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 avatar Aug 01 '13 09:08 Ahrengot

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.

Ahrengot avatar Aug 01 '13 10:08 Ahrengot

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!

polarblau avatar Aug 05 '13 19:08 polarblau