search

hootsuite / grid

Drag and drop library for two-dimensional, resizable and responsive lists
http://hootsuite.github.io/grid/
Apache License 2.0
3.57k stars 279 forks source link

removing reference to window.GridList which breaks if loaded via AMD #81

Closed adamcarr closed 8 years ago

billderose-zz commented 8 years ago

:+1: I am also unable to use gridList when loading the dependency via AMD.

NiGhTTraX commented 8 years ago

Can you please explain why loading via AMD fails?

@adamcarr I'm not comfortable with your fix. If GridList is not available then the code will fail with a ReferenceError error. That might be more confusing than the custom error thrown in that if block.

adamcarr commented 8 years ago

In this file, you have IIFE that is passed the function that acts as factory in the IIFE. The function that is passed takes 2 arguments, $ and GridList. These arguments are then used based on the closure scope in all jquery plugin uses. If you want to verify that window.GridList exists, which it only has to exist in the non-AMD (global) use, then check for its existence on line 8 before the call to factory as this is expecting a global GridList or window.GridList to exist.

NiGhTTraX commented 8 years ago

Ugh. Thanks @adamcarr for pointing that out. In that case, can you please remove the check altogether? It won't ever reach it if GridList is not defined because it'll throw at this line.

@skidding if we still want custom messages for missing dependencies we should move the checks to the module init code.

adamcarr commented 8 years ago

Done in latest commit.

NiGhTTraX commented 8 years ago

@adamcarr thanks.

@skidding Travis is not triggering again :sob: I ran the tests locally and everything's fine.

ovidiuch commented 8 years ago

:+1: