Conversation
|
+1 Is this going to be pulled yet? @imakewebthings |
|
+1, totally would save me time |
|
On first reading this looks good @summerisgone. Thank you. I'm currently in the middle of caring for a newborn so my time here comes in sporadic, short chunks. I'll try my best to use those chunks to test and merge this over the next few days. |
|
@imakewebthings my congrats for your baby! Stay happy and healthy! I'm ready to discuss any solution, I've tried to keep compatibility with previous builds as much as possible. |
|
@summerisgone The files you added to |
|
|
||
| var $ = window.jQuery | ||
| var Waypoint = window.Waypoint | ||
| var jquery = global.jQuery //require('jquery') |
There was a problem hiding this comment.
Is the comment here a remnant of an attempt to load jQuery via require, or a reminder to change it to require later at some point, or something else entirely?
There was a problem hiding this comment.
I tried to require jquery, but jquery's npm package doesn't contain $.forEach utilities and stuff. So this comment is more redundant. I still can try to shim jquery import.
|
@imakewebthings as I said, I excluded lib/* files from commit, to show only code difference for convenience. Added them now. |
|
@summerisgone Here's the plan. I've merged this PR into a branch, 5.0. I very much like this PR, and it makes it painfully obvious to me that a couple things should change in concert with this CommonJS changeset. The key ones:
Waypoint.extendJquery(jQuery)
jQuery('.blah').waypoint(...)I intend to do that work on top of your changes in the 5.0 branch. Simultaneously, there are some bug fixes and such that I would like to release as 4.x following master. Those changes will be folded into 5.0 through rebasing as I go along. |
|
@imakewebthings thank you for submission, I'm very glad again to contribute :) I have some questions about your plan:
|
|
@summerisgone The package.json main would point to an all-in-one build. If somebody were worried about file size and wanted to only include a subset of everything, there's nothing stoppng them from using For the jQuery/Zepto shortcuts, I intend to update those shortcuts to work without jQuery/Zepto. |
|
Awesome plan @imakewebthings I hope it will become reality soon. |
|
I agree so much. Waypoints is awesome, but using it with Webpack etc. can be a pain in the first point. CommonJS would solve it! |
|
If you need assistance from us @ webpack ensuring this can be bundled properly let me know. We are willing to help. |
|
Thank you for the offer @TheLarkInn, but the following official workaround is working pretty well for me. :) However, I would strongly vote for switching to a wider supported module in the next future. Maybe even consider es-modules via babel to supply all common module formats out there. Here is the workaround in case somebody else is looking for a fix: |
|
@axe312ger a PR on the readme would be really beneficial. |
Does that mean, that by default Waypoint won't use jQuery/Zepto or is it just the shortcuts? |
Here's my attempt to rewrite code in CommonJS modules. I've been using your waypoints a long time ago, and I'm happy to contribute.
This PR should solve issue #458. I didn't put files in lib/* because I don't like store code twice in repository. They can be easily added later.