FinalsClub / karmaworld

KarmaNotes.org v3.0
GNU Affero General Public License v3.0
7 stars 6 forks source link

move javascript snippets from templates into static javascript files #312

Closed btbonval closed 10 years ago

btbonval commented 10 years ago

Lots of javascript scattered about. 32 instances of <script in the templates files.

If a script needs to dynamically load data:

  1. main script could stay where it is
  2. the template could load the data into JSON within <script> but main script could be moved to a static file
  3. replace dynamic javascript with AJAX

If a script doesn't need to dynamically load data, then there isn't any reason for it to be in a template. The site will generally run faster if these are statically served.

charlesconnell commented 10 years ago

I'm not sure if it's true that the site will run faster with statically served scripts. General wisdom is that more HTTP requests means more work for the browser, unless we're talking about a particularly large javascript library that would be worth caching.

AndrewMagliozzi commented 10 years ago

This is a good task but not urgent at all. Jacob brought it up. Perhaps he can take the task at our next meeting.

btbonval commented 10 years ago

@charlesconnell and I discussed this briefly. In order to reduce HTTP requests, we should do our best to build a monolithic Javascript file and then minify it.

We both agree that AJAX is not a great answer because it is a lot of effort for very little value and increases the HTTP requests. Ideally we can load template variables into Javascript vars with <script> and otherwise consolidate the JS code.

There might be an issue of javascript which modifies DOM for a particular template. If there is a single JS file, we'll need to be careful about modifying the wrong DOM or modifying DOM that isn't there on other templates.

Maybe the issue of DOM-specific code could be rectified by naming all the javascript functions and calling the functions by name in the correct template?

btbonval commented 10 years ago

example of loading template var into js var

<script>
  var thingy = {{thingy}};
</script>
JHilker commented 10 years ago

I definitely agree that loading a single file + minifying it will be the best approach. Ideally the server could do this on the fly for production only, combining only the files that are required for the page. However if we can make a single massive file work site wide, that should be fine, and might be less work overall.

Options for getting python vars are script tags like you suggested, or embedding them as data attributes onto the DOM directly. Not sure which I actually prefer. On my last project we primarily did the later, and never had anything more than an includes on the source pages. But the script option might be cleaner overall.

I also definitely agree that we want to namespace all of our JS so that it can be concatenated easily. One of the modules I created for the edit form was created as such. I'm not sure I'm a fan of calling the functions explicitly by name in the templates though. What we did on my last project was pass the controller and action as data elements, then on every page call the same init script. The init script would then call a common script, and then the proper initializations based on the controller(page) and action. Overall I loved this setup, and would suggest we imitate it.

Main pieces of source code:

%body{:controller => controller_name, :action => action_name}

UTIL = {
  "exec": function( controller, action ) {
    var ns = WHERE;
    action = ( action === undefined ) ? "init" : action;

    if ( controller !== "" && ns[controller] && typeof ns[controller][action] == "function" ) {
      ns[controller][action]();
    }
  },

  "init": function() {
    var body = document.body,
        controller = body.getAttribute( "data-controller" ),
        action = body.getAttribute( "data-action" );

    UTIL.exec( "common" );
    UTIL.exec( controller );
    UTIL.exec( controller, action );
  }
};

$( UTIL.init );
btbonval commented 10 years ago

Good stuff. I'm only just getting into Javascript front-end dev stuff, so I don't know all the best practices. I think DOM is fine. It saves having to run the Javascript commands in <script> while still embedding the template variables.

I see how the namespacing is setup. It is an interesting idea.

Since we're planning to minify anyway, should we look into CoffeeScript? It might simplify the namespacing code with classes or something?

JHilker commented 10 years ago

I've never used CoffeeScript, and have heard mixed reviews. I'm up for learning it, but don't think is should be a requirement for this. I don't think it would make it that much simpler, and honestly doing a massive namespace change at the same time as converting all the JS to CoffeeScript may be a bit much at once. But if we want to do it I'm for it.

btbonval commented 10 years ago

I only suggested CoffeeScript because I seem to recall it allows fairly commonly repeated JavaScript memes to be simply and quickly written. I've looked into it a few times myself, and I agree with mixed reviews. The structure of the JavaScript looked like classes, which I believe CoffeeScript can handle.

CoffeeScript shouldn't be a requirement, though. Stretch goal if you get bored I guess ;)

AndrewMagliozzi commented 10 years ago

I have experience with it and don't love coffeescript. Vanilla JS does everything we need, which isn't too complex. Just my 2 cents.

On Feb 1, 2014, at 8:20 PM, Bryan Bonvallet notifications@github.com wrote:

I only suggested CoffeeScript because I seem to recall it allows fairly commonly repeated JavaScript memes to be simply and quickly written. I've looked into it a few times myself, and I agree with mixed reviews. The structure of the JavaScript looked like classes, which I believe CoffeeScript can handle.

CoffeeScript shouldn't be a requirement, though. Stretch goal if you get bored I guess ;)

— Reply to this email directly or view it on GitHub.

charlesconnell commented 10 years ago

This is done.