Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

fixed the demo

Because of $script.js's ultra fast loading it could happen that jQuery
wasn't defined so now gallery loads after jQuery is ready.  Also removed
empty line.
  • Loading branch information...
commit e2e66123926b8426cd18c016aa2a6919e654cd3c 1 parent 1f44d5c
Miha Rekar authored

Showing 3 changed files with 12 additions and 20 deletions. Show diff stats Hide diff stats

  1. +8 13 README.md
  2. +3 6 demo/index.html
  3. +1 1  gallery.js
21 README.md
Source Rendered
@@ -37,29 +37,24 @@ Example usage
37 37 (function() {
38 38 var images = ["images/image1.jpg", "images/image2.jpg", "images/image3.jpg"]; //array of paths to images
39 39 //load jQuery and gallery with $script.js - https://github.com/ded/script.js
40   - $script('https://ajax.googleapis.com/ajax/libs/jquery/1.6.2/jquery.min.js', 'jquery');
41   - $script('../gallery.js', 'gallery');
42   -
43   - $script.ready('gallery', function()
44   - {
45   - $script.domReady(function ()
46   - {
  40 + $script('https://ajax.googleapis.com/ajax/libs/jquery/1.6.2/jquery.min.js', function(){
  41 + $script('../gallery.js', function() {
47 42 //init the mrsi gallery and set the images with shuffle parameter set to true
48   - var gallery = new window.gallery(images, true);
  43 + var myGallery = new gallery(images, true);
49 44
50 45 //set menu controls
51 46 $('#toggle').click(function(){
52   - gallery.toggleType();
  47 + myGallery.toggleType();
53 48 });
54 49 $('#previous').click(function(){
55   - gallery.previous();
  50 + myGallery.previous();
56 51 });
57 52 $('#next').click(function(){
58   - gallery.next();
  53 + myGallery.next();
59 54 });
60   -
  55 +
61 56 //set keyboard shortcuts
62   - gallery.setKeyboardShortcuts();
  57 + myGallery.setKeyboardShortcuts();
63 58 });
64 59 });
65 60 })();
9 demo/index.html
@@ -26,11 +26,8 @@
26 26 }
27 27
28 28 //load jQuery and gallery with $script.js - https://github.com/ded/script.js
29   - $script('https://ajax.googleapis.com/ajax/libs/jquery/1.6.2/jquery.min.js', 'jquery');
30   - $script('../gallery.js', 'gallery');
31   -
32   - $script.ready('gallery', function() {
33   - jQuery(function () {
  29 + $script('https://ajax.googleapis.com/ajax/libs/jquery/1.6.2/jquery.min.js', function(){
  30 + $script('../gallery.js', function() {
34 31 //init the mrsi gallery and set the images with shuffle parameter set to true
35 32 var myGallery = new gallery(images, true);
36 33
@@ -44,7 +41,7 @@
44 41 $('#next').click(function(){
45 42 myGallery.next();
46 43 });
47   -
  44 +
48 45 //set keyboard shortcuts
49 46 myGallery.setKeyboardShortcuts();
50 47 });
2  gallery.js
@@ -148,4 +148,4 @@

21 comments on commit e2e6612

Alpheus

Actually, jQuery.ready will fire even after the ready state. Can't reproduce what you've written in the commit message. Did you clear your cache before testing?

Miha Rekar
Owner

Yup, I cleared it and this fixed it. Happened with Chrome and Firefox every time i cleared the cache and loaded it fresh. This first loads jQuery and when it's loaded it loads gallery and when gallery is loaded it inits the gallery and executes all that code, so I think I don't need domready or jQuery ready. Correct me if im wrong...

Alpheus
$script('https://ajax.googleapis.com/ajax/libs/jquery/1.6.2/jquery.min.js', 'jquery');
$script('../gallery.js', 'gallery');
$script.ready('gallery', function() {

Allows the javascript files to be downloaded in parallel, right now you're downloading them sequentially, even worse - with a slight delay because of the callback function.

Miha Rekar
Owner

Yeah, but gallery REQUIRES jQuery and if jQuery is not loaded then there is an error that jQuery is not defined. It should be a plugin...

Alpheus

My understanding was the $script handles the download and execution separately (how else would callbacks make sense?). This does not seem to be the case. Use RequireJS instead ;)

Miha Rekar
Owner

$script.js loads everything in parallel so gallery could load (and executes because of its small size) before jQuery does so it errors. $script.js fires callback after load AND execution.

Klemen Slavič

Why complicate stuff? Just include three script tags before </body>, use $(function() { ... }); and be done with it. In my experience, using loader scripts only does damage to the usefulness and explanatory value of the demo.

If anyone needs to load scripts on-demand, they'll invest the time to implement any of the available options, like http://requirejs.org/.

IMHO, the simpler you can make the demo, the better. If the demo code for the simplest case seems to be too complicated, it means you need to work more on your library and API. Rinse, repeat.

Miha Rekar
Owner

Good point...on it :D

Alpheus

Makes sense, I'll remove the $script wrappers on my end and keep it clean as well. Any thoughts on the API yet?

Miha Rekar
Owner

Done. API is probably better way to do it so maybe it should be done... :)

Klemen Slavič

Err... I meant, do you want to continue with the current code base (class-like behaviour) or apply the module pattern, as suggested by @Alpheus?

Miha Rekar
Owner

Being n00b and all - WWJD?

Alpheus

What would Miha do?

Klemen Slavič

I'd suggest keeping the current version in master and creating a dev branch to reimplement the current class-based form into the module pattern. That way, you can develop in parallel and later replace the master with dev while keeping the existing API as close as possible (possibly identical) to the one used in the demo.

Let the demo be the spec of your API while you reimplement the code.

Miha Rekar
Owner

Goods and bads?

Klemen Slavič

The good is that with the module pattern, people using your code won't forget to add the new keyword which your current code requires (not using it will break the code in weird ways). It leads to better coding practices and better control over the visibility and explicitness of the exported API methods.

See http://www.adequatelygood.com/2010/3/JavaScript-Module-Pattern-In-Depth

The bad is outlined is Jonathan Snook's blog post, but you probably won't get as far as he did regarding debugging anyways, since the gripes he deals with all have to do with module inheritance, which you don't have:

http://snook.ca/archives/javascript/no-love-for-module-pattern

All in all, both approaches are something you should look into and learn, but I suggest learning and implementing the module pattern first, since it's easier to manage and understand. Look at @Alpheus' code base for how he implemented it as a guide to editing it yourself.

Alpheus

Prototype-based construction will allow developers to introspect the public API, the factory method allows you to be more liberal at refactoring (e.g. changing constructor structure, parameters, parameter order, etc.).

Constructing the object tree in the constructor like you did originally allows you to be leaner on the architecture but ultimately it incurs certain performance constraints (every new object has different copies of their respective class methods). The danger lies in people not identifying the constructor as a 'constructor' (i.e. something that must be called with new), thus messing with the currently active object in scope (this, usually dom nodes or window)

Alpheus

@KrofDrakula thanks for the second link, JS module inheritance is a topic I'm very interested in.

Alpheus

@KrofDrakula: thanks, already read it ages ago :) Crockford is an amazing speaker.

Please sign in to comment.
Something went wrong with that request. Please try again.