yellow team mailing list archive

[Thiago Veronezi <thiago@xxxxxxxxxxxx>]

Thiago Veronezi has proposed merging lp:~tveronezi/juju-gui/combinecss into lp:juju-gui.

Requested reviews:
  Juju GUI Hackers (juju-gui)
Related bugs:
  Bug #1078293 in juju-ui: "Resource minification/aggregation css"
  https://bugs.launchpad.net/juju-gui/+bug/1078293

For more details, see:
https://code.launchpad.net/~tveronezi/juju-gui/combinecss/+merge/134140

Resource minification/aggregation css

* We should aggregate and minimize the css sources in order to improve the load speed of the application;
* We dont want to use the yui combo loader feature;
* The final product will provide three css files: one for the static dependencies, one for our generated templates and one our generated sprites.
-- 
https://code.launchpad.net/~tveronezi/juju-gui/combinecss/+merge/134140
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~tveronezi/juju-gui/combinecss into lp:juju-gui.

=== modified file '.bzrignore'
--- .bzrignore	2012-11-06 21:44:38 +0000
+++ .bzrignore	2012-11-13 15:49:26 +0000
@@ -21,3 +21,4 @@
 app/assets/manifest.appcache
 app/assets/sprite
 app/assets/javascripts/generated/
+app/assets/stylesheets/all-static.css

=== modified file 'Makefile'
--- Makefile	2012-11-09 15:37:08 +0000
+++ Makefile	2012-11-13 15:49:26 +0000
@@ -17,7 +17,8 @@
 COMPRESSED_FILES=app/assets/javascripts/generated/all-app-debug.js \
 	app/assets/javascripts/generated/all-app.js \
 	app/assets/javascripts/generated/all-third.js \
-	app/assets/javascripts/generated/all-yui.js
+	app/assets/javascripts/generated/all-yui.js \
+	app/assets/stylesheets/all-static.css
 DATE=$(shell date -u)
 APPCACHE=app/assets/manifest.appcache
 
@@ -67,6 +68,7 @@
 spritegen: $(SPRITE_GENERATED_FILES)
 
 $(COMPRESSED_FILES): node_modules/yui node_modules/d3/d3.v2.min.js $(JSFILES)
+	@rm -f app/assets/stylesheets/all-static.css
 	@rm -Rf app/assets/javascripts/generated/
 	@mkdir app/assets/javascripts/generated/
 	@./bin/merge-files
@@ -92,6 +94,7 @@
 	@rm -Rf bin/sprite/
 	@rm -Rf app/assets/sprite/
 	@rm -Rf app/assets/javascripts/generated/
+	@rm -f app/assets/stylesheets/all-static.css
 
 $(APPCACHE): manifest.appcache.in
 	@cp manifest.appcache.in $(APPCACHE)

=== modified file 'app/index.html'
--- app/index.html	2012-11-12 14:46:20 +0000
+++ app/index.html	2012-11-13 15:49:26 +0000
@@ -7,9 +7,7 @@
     <meta name="description" content="">
     <meta name="author" content="Juju team">
     <link href='http://fonts.googleapis.com/css?family=Ubuntu:400,400italic&subset=latin,latin-ext' rel='stylesheet' type='text/css'>
-    <link rel="stylesheet" href="/juju-ui/assets/stylesheets/bootstrap-2.0.4.css">
-    <link rel="stylesheet"
-          href="/juju-ui/assets/stylesheets/bootstrap-responsive-2.0.4.css">
+    <link rel="stylesheet" href="/assets/stylesheets/all-static.css">
     <link rel="stylesheet" href="/juju-ui/assets/stylesheets/juju-gui.css">
     <link rel="stylesheet" href="/juju-ui/assets/sprite/sprite.css">
     <!--[if lt IE 9]>

=== modified file 'bin/merge-files'
--- bin/merge-files	2012-11-13 13:55:16 +0000
+++ bin/merge-files	2012-11-13 15:49:26 +0000
@@ -29,7 +29,9 @@
   var merge = require('../lib/merge-files.js'),
       syspath = require('path'),
       paths,
-      reqs;
+      reqs,
+      filesToLoad,
+      thirdPartyCss;
 
   // First we find all of the paths to our custom Javascript in the app
   // directory.  We need to tell the function to ignore the "assets" directory
@@ -37,7 +39,8 @@
   // "syspath.join(process.cwd(), ...)" or else I have... "Error: Cannot find
   // module './app/config.js'" from node's internal module.js file.
   paths = merge.readdir(syspath.join(process.cwd(), './app'),
-    [syspath.join(process.cwd(), './app/assets'), './app/modules-debug.js']);
+    [syspath.join(process.cwd(), './app/assets'),
+     syspath.join(process.cwd(), './app/modules-debug.js')]);
 
   // Get the name of all the YUI modules that our custom js files use directly.
   reqs = merge.loadRequires(paths);
@@ -50,15 +53,26 @@
   reqs.push('gallery-markdown');
   reqs.push('loader-base');
 
-  // Get all of the YUI files and their dependencies, and combine them.
-  merge.combine(merge.getYUIFiles(reqs),
-      './app/assets/javascripts/generated/all-yui.js', true);
+  // Get all of the YUI files and their dependencies
+  filesToLoad = merge.getYUIFiles(reqs);
+  thirdPartyCss = filesToLoad.css;
+  thirdPartyCss.push('./app/assets/stylesheets/bootstrap-2.0.4.css');
+  thirdPartyCss.push('./app/assets/stylesheets/bootstrap-responsive-2.0.4.css');
+  console.log('CSS: ' + thirdPartyCss);
+
+  // Combine YUI js files
+  merge.combine(filesToLoad.js,
+      './app/assets/javascripts/generated/all-yui.js', 'uglifyjs');
+
+  // Combine third party css files
+  merge.combine(thirdPartyCss,
+      './app/assets/stylesheets/all-static.css', 'yui-css');
 
   // Combine third party js libraries
   merge.combine([ './app/assets/javascripts/d3.v2.min.js',
       './app/assets/javascripts/reconnecting-websocket.js',
       './app/assets/javascripts/svg-layouts.js'],
-      './app/assets/javascripts/generated/all-third.js', true);
+      './app/assets/javascripts/generated/all-third.js', 'uglifyjs');
 
   // Now we only need to generate the file that is used to tell YUI where all
   // the dependencies are.  We either use a debug version or the production
@@ -66,10 +80,11 @@
   // The production version aggregates all of the files together.
 
   // Creating the combined file for the modules-debug.js and config.js files
-  merge.combine([ './app/modules-debug.js', './app/config.js'],
-          './app/assets/javascripts/generated/all-app-debug.js', false);
+  merge.combine(['./app/modules-debug.js', './app/config.js'],
+      './app/assets/javascripts/generated/all-app-debug.js');
 
   // Creating the combined file for all our files.  Note that this includes
-  // app/modules.js in the rollup, which is why that file still needs to exist.
-  merge.combine(paths, './app/assets/javascripts/generated/all-app.js', true);
+  // app/modules.js in the rollup, which is why that file still needs exist.
+  merge.combine(paths, './app/assets/javascripts/generated/all-app.js',
+      'uglifyjs');
 });

=== modified file 'lib/merge-files.js'
--- lib/merge-files.js	2012-11-09 15:37:08 +0000
+++ lib/merge-files.js	2012-11-13 15:49:26 +0000
@@ -10,9 +10,9 @@
       syspath = require('path'),
       compressor = require('node-minify');
 
-  function minify(file) {
+  function minify(file, compressorType) {
     var execution = new compressor.minify(
-        { type: 'uglifyjs',
+        { type: compressorType,
           fileIn: file,
           fileOut: file,
           callback: function(err) {
@@ -21,19 +21,18 @@
             }
           }});
   }
-  exports.minify = minify;
 
   // It combines the files defined by "files" into a single (compressed or not)
   // js file.
-  function combine(files, outputFile, shouldMinify) {
+  function combine(files, outputFile, compressorType) {
     var str = [];
     Y.Array.each(files, function(file) {
       console.log('file -> ' + file);
       str.push(fs.readFileSync(file, 'utf8'));
     });
     fs.writeFileSync(outputFile, str.join('\n'), 'utf8');
-    if (shouldMinify) {
-      minify(outputFile);
+    if (compressorType) {
+      minify(outputFile, compressorType);
     }
   }
   exports.combine = combine;
@@ -131,7 +130,7 @@
       ignoreRegistered: true,
       require: reqs});
     out = loader.resolve(true);
-    return out.js;
+    return out;
   }
   exports.getYUIFiles = getYUIFiles;
 });

=== modified file 'lib/server.js'
--- lib/server.js	2012-11-09 15:37:08 +0000
+++ lib/server.js	2012-11-13 15:49:26 +0000
@@ -11,6 +11,29 @@
     view = require('./view.js');
 
 
+/**
+ * It finds a file under the given path. The first match wins.
+ * @param {string} path starting point.
+ * @param {string} fileName name of the file to be found.
+ * @return {string} the file path.
+ */
+function findFile(path, fileName) {
+  var file, files = fs.readdirSync(path);
+  for (var i = 0; i < files.length; i = i + 1) {
+    if (files[i] === fileName) {
+      return path + '/' + files[i];
+    }
+    file = fs.statSync(path + '/' + files[i]);
+    if (file.isDirectory()) {
+      file = findFile(path + '/' + files[i], fileName);
+      if (file) {
+        return file;
+      }
+    }
+  }
+  return null;
+}
+
 server.configure(function() {
   server.set('views', __dirname + './lib/views/');
   server.set('view engine', 'handlebars');
@@ -51,6 +74,25 @@
   res.sendfile('app/assets/javascripts/generated/all-third.js');
 });
 
+server.get('/assets/stylesheets/:file', function(req, res) {
+  var file, fileName = req.params.file;
+  if (path.extname(fileName).toLowerCase() === '.css') {
+    res.sendfile('app/assets/stylesheets/' + fileName);
+  } else {
+    // We've merged all the yui and third party files into a single css file.
+    // YUI expects to load its images from the same path as its css files, so
+    // we need to mock this position. When the system tries to load a resource
+    // from "app/assets/stylesheets/", it tries to find this resource under the
+    // "./app/assets" directory. The first match wins. Another solution would
+    // be to manually copy all the images we need to the
+    // "./app/assets/stylesheets" directory.
+    // This is an example of image...
+    // ./yui/datatable-sort/assets/skins/sam/sort-arrow-sprite-ie.png
+    file = findFile('./app/assets', fileName);
+    res.sendfile(file);
+  }
+});
+
 server.get('/assets/modules.js', function(req, res) {
   if (debugMode) {
     res.sendfile('app/assets/javascripts/generated/all-app-debug.js');