Added AMD support #124
Added AMD support #124
Conversation
| (function($, factory){ | ||
| 'use strict'; | ||
| if (typeof define === 'function' && define.amd) { | ||
| return factory($); |
There was a problem hiding this comment.
I think we should rewrite this line to define(['jquery'], factory);.
| } else { | ||
| window.jScroll = factory($); | ||
| } | ||
| })(jQuery || null, function($) { |
There was a problem hiding this comment.
jQuery || null is pretty useless, isn't it?
| if (typeof define === 'function' && define.amd) { | ||
| return factory($); | ||
| } else { | ||
| window.jScroll = factory($); |
There was a problem hiding this comment.
Do we really like to make a global variable for jScroll?
There was a problem hiding this comment.
Yeah. To support the current singleton approach
There was a problem hiding this comment.
I think this is not needed, because it's a jquery plugin and is therefore attached to jquery, see https://github.com/pklauzinski/jscroll/blob/master/jquery.jscroll.js#L214
|
Can you add "fix #114" to the pr description in order to link this pr properly to the issue? |
|
fix #114 |
|
Done
…On Mon, Mar 27, 2017 at 6:27 PM, Klaus ***@***.***> wrote:
Can you add "fix #114 <#114>"
to link this pr properly to the issue?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#124 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AWbVF7YBjpwPfQRf-Cjyqfly1WN9pRUEks5rp7IigaJpZM4MBU79>
.
|
|
Hey guys, what about this PR? I would need it for a project using npm and webpack |
|
i'm using the branch from @gs-rpal because I need CommonJS support for webpack. I've put the |
|
@Dotmagic I managed to have the exact need, released a fork version that you may try it out. |
|
@ratson I'm not sure its a good idea to create a fork with almost same name but lower the version to 0.0.2? I think its confusing. @pklauzinski is still active on this project and it looks like he still welcome contributions. I'm using CommonJS or ES6 imports now, I believe we should enhance the pull request to support all this types. Soon bower will disappear and we are going to manage scripts with yarn/webpack, for that reason I consider this feature essential. |
Added AMD Support (fix #114)