Repository: vega/vega-lite
Pull request #710 (2015-11-07 22:39:22)

Refactor axis to minimize side effects
- All produced properties that have special rules are organized in methods with the following patterns: `axis.<propertyName>` and `axis.properties.<propertyGroup>`
- #181 now support explicit specification of all Vega’s __axis properties__ (`'format', 'grid', 'offset', 'orient', 'tickSize', 'ticks', 'title', ‘titleOffset', 'layer', 'tickPadding', 'tickSize', 'tickSizeMajor', 'tickSizeMinor', 'tickSizeEnd',
    'values', ‘subdivide’`) and __property groups__ (`'axis', 'grid', 'labels', 'title', 'ticks', 'majorTicks', 'minorTicks’`)
- Output spec should remain the same except
  - `titleOffset` is set to `undefined` (auto) for `x` and `y` — so we can just rely on the [new Vega’s axis auto title offset](https://github.com/vega/vega/pull/421) — __This mean we need to wait until Vega releases new version before we can release one.__


Patch:
diff --git a/src/compiler/axis.js b/src/compiler/axis.js
index 1124888fd..d56298ab5 100644
--- a/src/compiler/axis.js
+++ b/src/compiler/axis.js
@@ -3,101 +3,198 @@
 require('../globals');
 
 var util = require('../util'),
-  setter = util.setter,
-  getter = util.getter,
   time = require('./time');
 
 var axis = module.exports = {};
 
-axis.def = function(name, encoding, layout, stats, opt) {
+axis.def = function(name, encoding, layout, stats) {
   var isCol = name == COL,
     isRow = name == ROW,
     type = isCol ? 'x' : isRow ? 'y' : name;
 
-  // TODO: rename def to axisDef and avoid side effects where possible.
-
   var def = {
     type: type,
-    scale: name,
-    properties: {},
-    layer: encoding.encDef(name).axis.layer
+    scale: name
   };
 
-  var orient = axis.orient(encoding, name, stats);
-  if (orient) {
-    def.orient = orient;
-  }
+  // Add optional properties
+  [
+    // properties with special rules (so it has axis[property] methods) -- call rule functions
+    'format', 'grid', 'offset', 'orient', 'tickSize', 'ticks', 'title', 'titleOffset',
+    // Otherwise, only produce default values in the schema, or explicit value if specified
+    'layer', 'tickPadding', 'tickSize', 'tickSizeMajor', 'tickSizeMinor', 'tickSizeEnd',
+    'values', 'subdivide'
+  ].forEach(function(property) {
+    var value = axis[property] ?
+          axis[property](encoding, name, layout, stats) :
+          encoding.encDef(name).axis[property];
+    if (value !== undefined) {
+      def[property] = value;
+    }
+  });
+
+  // Add properties under axis.properties
+  var properties = encoding.encDef(name).axis.properties || {};
+
+  [
+    'axis', 'grid', 'labels', 'title', // have special rules
+    'ticks', 'majorTicks', 'minorTicks' // only default values
+  ].forEach(function(property) {
+    var value = axis.properties[property] ?
+      axis.properties[property](encoding, name, properties[property], layout, def) :
+      properties[property];
+    if (value !== undefined) {
+      def.properties = def.properties || {};
+      def.properties[property] = value;
+    }
+  });
 
-  // Add axis label custom scale (for bin / time)
-  def = axis.labels.scale(def, encoding, name);
-  def = axis.labels.format(def, encoding, name);
-  def = axis.labels.angle(def, encoding, name);
+  return def;
+};
 
-  // for x-axis, set ticks for Q or rotate scale for ordinal scale
-  if (name == X) {
-    if ((encoding.isDimension(X) || encoding.isType(X, T)) &&
-        !('angle' in getter(def, ['properties', 'labels']))) {
-      // TODO(kanitw): Jul 19, 2015 - #506 add condition for rotation
-      def = axis.labels.rotate(def);
-    } else { // Q
-      def.ticks = encoding.encDef(name).axis.ticks;
-    }
+axis.format = function(encoding, name) {
+  var format = encoding.encDef(name).axis.format;
+  if (format !== undefined)  {
+    return format;
   }
 
-  // TitleOffset depends on labels rotation
-  def.titleOffset = axis.titleOffset(encoding, layout, name);
+  if (encoding.isType(name, Q)) {
+    return encoding.numberFormat(name);
+  } else if (encoding.isType(name, T)) {
+    var timeUnit = encoding.encDef(name).timeUnit;
+    if (!timeUnit) {
+      return encoding.config('timeFormat');
+    } else if (timeUnit === 'year') {
+      return 'd';
+    }
+  }
+  return undefined;
+};
 
-  //def.offset is used in axis.grid
-  if(isRow) def.offset = axis.titleOffset(encoding, layout, Y) + 20;
-  // FIXME(kanitw): Jul 19, 2015 - offset for column when x is put on top
+axis.grid = function(encoding, name) {
+  var grid = encoding.axis(name).grid;
+  if (grid !== undefined) {
+    return grid;
+  }
 
-  def = axis.grid(def, encoding, name, layout);
-  def = axis.title(def, encoding, name, layout, opt);
+  // If `grid` is unspecified, the default value is `true` for
+  // - ROW and COL.
+  // - X and Y that have (1) quantitative fields that are not binned or (2) time fields.
+  // Otherwise, the default value is `false`.
+  return name === ROW || name === COL ||
+    (encoding.isTypes(name, [Q, T]) && !encoding.encDef(name).bin);
+};
 
-  if (isRow || isCol) {
-    def = axis.hideTicks(def);
+axis.offset = function(encoding, name, layout) {
+  var offset = encoding.encDef(name).axis.offset;
+  if (offset) {
+    return offset;
   }
 
-  return def;
+  if(name === ROW) {
+   return layout.y.axisTitleOffset + 20;
+  }
+  return undefined;
 };
 
-axis.orient = function(encoding, name, stats) {
+axis.orient = function(encoding, name, layout, stats) {
   var orient = encoding.encDef(name).axis.orient;
   if (orient) {
     return orient;
   } else if (name === COL) {
     return 'top';
   } else if (name === X && encoding.has(Y) && encoding.isOrdinalScale(Y) && encoding.cardinality(Y, stats) > 30) {
+    // FIXME remove this case and migrate this logic to vega-lite-ui
     // x-axis for long y - put on top
     return 'top';
   }
   return undefined;
 };
 
-axis.grid = function(def, encoding, name, layout) {
-  var cellPadding = layout.cellPadding,
-    isCol = name == COL,
-    isRow = name == ROW;
+axis.ticks = function(encoding, name) {
+  var ticks = encoding.encDef(name).axis.ticks;
+  if (ticks !== undefined) {
+    return ticks;
+  }
 
-  var _grid = encoding.axis(name).grid;
+  // FIXME depends on scale type too
+  if (name === X && !encoding.encDef(name).bin) {
+    return 5;
+  }
 
-  // If `grid` is unspecified, the default value is `true` for ROW and COL.
-  // For X and Y, the default value is `true` for (1) quantitative fields that are not binned and (2) time fields.
-  // Otherwise, the default value is `false`.
-  var grid = _grid === undefined ?
-    ( name === ROW || name === COL ||
-      (encoding.isTypes(name, [Q, T]) && !encoding.encDef(name).bin)
-    ) : _grid;
+  return undefined;
+};
+
+axis.tickSize = function(encoding, name) {
+  var tickSize = encoding.encDef(name).axis.tickSize;
+  if (tickSize !== undefined) {
+    return tickSize;
+  }
+  if (name === ROW || name === COL) {
+    return 0;
+  }
+  return undefined;
+};
+
+
+axis.title = function (encoding, name, layout) {
+  var axisSpec = encoding.encDef(name).axis;
+  if (axisSpec.title !== undefined) {
+    return axisSpec.title;
+  }
+
+  // if not defined, automatically determine axis title from field def
+  var fieldTitle = encoding.fieldTitle(name);
+
+  var maxLength;
+  if (axisSpec.titleMaxLength) {
+  maxLength = axisSpec.titleMaxLength;
+  } else if (name === X) {
+    maxLength = layout.cellWidth / encoding.config('characterWidth');
+  } else if (name === Y) {
+    maxLength = layout.cellHeight / encoding.config('characterWidth');
+  }
 
-  if (grid) {
-    def.grid = true;
+  return maxLength ? util.truncate(fieldTitle, maxLength) : fieldTitle;
+};
+
+
+axis.titleOffset = function (encoding, name) {
+  // return specified value if specified
+  var value = encoding.axis(name).titleOffset;
+  if (value)  return value;
+
+  switch (name) {
+    case ROW: return 0;
+    case COL: return 35;
+  }
+  return undefined;
+};
 
-    if (isCol) {
+// PROPERTIES
+
+axis.properties = {};
+
+axis.properties.axis = function(encoding, name, spec) {
+  if (name === ROW || name === COL) {
+    // hide axis for facets
+    return util.extend({
+      opacity: {value: 0}
+    }, spec || {});
+  }
+  return spec || undefined;
+};
+
+axis.properties.grid = function(encoding, name, spec, layout, def) {
+  var cellPadding = layout.cellPadding;
+
+  if (def.grid) {
+    if (name == COL) {
       // set grid property -- put the lines on the right the cell
       var yOffset = encoding.config('cellGridOffset');
 
       // TODO(#677): this should depend on orient
-      def.properties.grid = {
+      return util.extend({
         x: {
           offset: layout.cellWidth * (1+ cellPadding/2.0),
           // default value(s) -- vega doesn't do recursive merge
@@ -113,13 +210,13 @@ axis.grid = function(def, encoding, name, layout) {
         },
         stroke: { value: encoding.config('cellGridColor') },
         strokeOpacity: { value: encoding.config('cellGridOpacity') }
-      };
-    } else if (isRow) {
+      }, spec || {});
+    } else if (name == ROW) {
       var xOffset = encoding.config('cellGridOffset');
 
       // TODO(#677): this should depend on orient
       // set grid property -- put the lines on the top
-      def.properties.grid = {
+      return util.extend({
         y: {
           offset: -layout.cellHeight * (cellPadding/2),
           // default value(s) -- vega doesn't do recursive merge
@@ -137,121 +234,56 @@ axis.grid = function(def, encoding, name, layout) {
         },
         stroke: { value: encoding.config('cellGridColor') },
         strokeOpacity: { value: encoding.config('cellGridOpacity') }
-      };
+      }, spec || {});
     } else {
-      def.properties.grid = {
+      return util.extend({
         stroke: { value: encoding.config('gridColor') },
         strokeOpacity: { value: encoding.config('gridOpacity') }
-      };
+      }, spec || {});
     }
   }
-  return def;
+  return spec || undefined;
 };
 
-axis.hideTicks = function(def) {
-  def.properties.ticks = {opacity: {value: 0}};
-  def.properties.majorTicks = {opacity: {value: 0}};
-  def.properties.axis = {opacity: {value: 0}};
-  return def;
-};
+axis.properties.labels = function(encoding, name, spec, layout, def) {
+  var timeUnit = encoding.encDef(name).timeUnit;
+  if (encoding.isType(name, T) && timeUnit && (time.hasScale(timeUnit))) {
+    spec = util.extend({
+      text: {scale: 'time-' + timeUnit}
+    }, spec || {});
+  }
 
-axis.title = function (def, encoding, name, layout) {
-  var ax = encoding.encDef(name).axis;
-
-  if (ax.title) {
-    def.title = ax.title;
-  } else {
-    // if not defined, automatically determine axis title from field def
-    var fieldTitle = encoding.fieldTitle(name),
-      maxLength;
-
-    if (ax.titleMaxLength) {
-      maxLength = ax.titleMaxLength;
-    } else if (name===X) {
-      maxLength = layout.cellWidth / encoding.config('characterWidth');
-    } else if (name === Y) {
-      maxLength = layout.cellHeight / encoding.config('characterWidth');
-    }
+  if (encoding.isTypes(name, [N, O]) && encoding.axis(name).labelMaxLength) {
+    // TODO replace this with Vega's labelMaxLength once it is introduced
+    spec = util.extend({
+      text: {
+        template: '{{ datum.data | truncate:' + encoding.axis(name).labelMaxLength + '}}'
+      }
+    }, spec || {});
+  }
 
-    def.title = maxLength ? util.truncate(fieldTitle, maxLength) : fieldTitle;
+   // for x-axis, set ticks for Q or rotate scale for ordinal scale
+  if (name == X) {
+    if ((encoding.isDimension(X) || encoding.isType(X, T))) {
+      spec = util.extend({
+        angle: {value: 270},
+        align: {value: def.orient === 'top' ? 'left': 'right'},
+        baseline: {value: 'middle'}
+      }, spec || {});
+    }
   }
+  return spec || undefined;
+};
 
+axis.properties.title = function(encoding, name, spec, layout) {
   if (name === ROW) {
-    def.properties.title = {
+    return util.extend({
       angle: {value: 0},
       align: {value: 'right'},
       baseline: {value: 'middle'},
-      dy: {value: (-layout.height/2) -20}
-    };
+      dy: {value: (-layout.height / 2) - 20}
+    }, spec || {});
   }
-
-  return def;
-};
-
-axis.labels = {};
-
-/** add custom label for time type and bin */
-axis.labels.scale = function(def, encoding, name) {
-  // time
-  var timeUnit = encoding.encDef(name).timeUnit;
-  if (encoding.isType(name, T) && timeUnit && (time.hasScale(timeUnit))) {
-    setter(def, ['properties','labels','text','scale'], 'time-'+ timeUnit);
-  }
-  // FIXME bin
-  return def;
+  return spec || undefined;
 };
 
-/**
- * Determine number format or truncate if maxLabel length is presented.
- */
-axis.labels.format = function (def, encoding, name) {
-  if (encoding.axis(name).format) {
-    def.format = encoding.axis(name).format;
-  } else if (encoding.isType(name, Q)) {
-    def.format = encoding.numberFormat(name);
-  } else if (encoding.isType(name, T)) {
-    var timeUnit = encoding.encDef(name).timeUnit;
-    if (!timeUnit) {
-      def.format = encoding.config('timeFormat');
-    } else if (timeUnit === 'year') {
-      def.format = 'd';
-    }
-  } else if (encoding.isTypes(name, [N, O]) && encoding.axis(name).labelMaxLength) {
-    setter(def,
-      ['properties','labels','text','template'],
-      '{{ datum.data | truncate:' +
-      encoding.axis(name).labelMaxLength + '}}'
-    );
-  }
-
-  return def;
-};
-
-axis.labels.angle = function(def, encoding, name) {
-  var angle = encoding.axis(name).labelAngle;
-  if (typeof angle === 'undefined') return def;
-
-  setter(def, ['properties', 'labels', 'angle', 'value'], angle);
-  return def;
-};
-
-axis.labels.rotate = function(def) {
- var align = def.orient ==='top' ? 'left' : 'right';
- setter(def, ['properties','labels', 'angle', 'value'], 270);
- setter(def, ['properties','labels', 'align', 'value'], align);
- setter(def, ['properties','labels', 'baseline', 'value'], 'middle');
- return def;
-};
-
-axis.titleOffset = function (encoding, layout, name) {
-  // return specified value if specified
-  var value = encoding.axis(name).titleOffset;
-  if (value)  return value;
-
-  switch (name) {
-    //FIXME make this adjustable
-    case ROW: return 0;
-    case COL: return 35;
-  }
-  return getter(layout, [name, 'axisTitleOffset']);
-};
diff --git a/src/schema/schema.js b/src/schema/schema.js
index a6d4480b6..9795ed3c1 100644
--- a/src/schema/schema.js
+++ b/src/schema/schema.js
@@ -257,7 +257,7 @@ var axisMixin = {
         },
         ticks: {
           type: 'integer',
-          default: 5,
+          default: undefined,
           minimum: 0,
           description: 'A desired number of ticks, for axes visualizing quantitative scales. The resulting number may be different so that values are "nice" (multiples of 2, 5, 10) and lie within the underlying scale\'s range.'
         },
@@ -274,13 +274,6 @@ var axisMixin = {
           minimum: 0,
           description: 'Truncate labels that are too long.'
         },
-        labelAngle: {
-          type: 'integer',
-          default: undefined, // auto
-          minimum: 0,
-          maximum: 360,
-          description: 'Angle by which to rotate labels. Set to 0 to force horizontal.'
-        },
         titleMaxLength: {
           type: 'integer',
           default: undefined,


Test patch:
diff --git a/test/compiler/axis.test.js b/test/compiler/axis.test.js
index 11e3de3d1..13a1c5ecb 100644
--- a/test/compiler/axis.test.js
+++ b/test/compiler/axis.test.js
@@ -48,49 +48,13 @@ describe('Axis', function() {
     // FIXME(kanitw): Jul 19, 2015 - write test
   });
 
-  describe('hideTicks()', function () {
-    var def = axis.hideTicks({properties:{}});
-    it('should adjust ticks', function () {
-      expect(def.properties.ticks).to.eql({opacity: {value: 0}});
-    });
-    it('should adjust majorTicks', function () {
-      expect(def.properties.majorTicks).to.eql({opacity: {value: 0}});
-    });
-    it('should adjust axis', function () {
-      expect(def.properties.axis).to.eql({opacity: {value: 0}});
-    });
-  });
-
-  describe('labels.scale()', function () {
-    // FIXME(kanitw): Jul 19, 2015 - write test
-  });
-
-  describe('labels.format()', function () {
-    // FIXME(kanitw): Jul 19, 2015 - write test
-  });
-
-  describe('labels.angle()', function () {
-    it('should set explicitly specified angle', function () {
-      var def = axis.labels.angle({}, Encoding.fromSpec({
-        encoding: {
-          x: {name: 'a', type: 'T', axis:{labelAngle: 90}}
-        }
-      }), 'x');
-      expect(def.properties.labels.angle).to.eql({value: 90});
-    });
-  });
-
-  describe('labels.rotate()', function () {
-    // FIXME(kanitw): Jul 19, 2015 - write test
-  });
-
   describe('orient()', function () {
     it('should return specified orient', function () {
       var orient = axis.orient(Encoding.fromSpec({
           encoding: {
             x: {name: 'a', axis:{orient: 'bottom'}}
           }
-        }), 'x', stats);
+        }), 'x', {}, stats);
       expect(orient).to.eql('bottom');
     });
 
@@ -99,7 +63,7 @@ describe('Axis', function() {
           encoding: {
             x: {name: 'a'}
           }
-        }), 'x', stats);
+        }), 'x', {}, stats);
       expect(orient).to.eql(undefined);
     });
 
@@ -109,7 +73,7 @@ describe('Axis', function() {
             x: {name: 'a'},
             col: {name: 'a'}
           }
-        }), 'col', stats);
+        }), 'col', {}, stats);
       expect(orient).to.eql('top');
     });
 
@@ -119,56 +83,56 @@ describe('Axis', function() {
             x: {name: 'a'},
             y: {name: 'b', type: 'O'}
           }
-        }), 'x', stats);
+        }), 'x', {}, stats);
       expect(orient).to.eql('top');
     });
   });
 
   describe('title()', function () {
     it('should add explicitly specified title', function () {
-      var def = axis.title({}, Encoding.fromSpec({
+      var title = axis.title(Encoding.fromSpec({
           encoding: {
             x: {name: 'a', axis: {title: 'Custom'}}
           }
         }), 'x', stats, layout);
-      expect(def.title).to.eql('Custom');
+      expect(title).to.eql('Custom');
     });
 
     it('should add return fieldTitle by default', function () {
-      var def = axis.title({}, Encoding.fromSpec({
+      var title = axis.title(Encoding.fromSpec({
           encoding: {
             x: {name: 'a', axis: {titleMaxLength: '3'}}
           }
         }), 'x', layout);
-      expect(def.title).to.eql('a');
+      expect(title).to.eql('a');
     });
 
     it('should add return fieldTitle by default', function () {
-      var def = axis.title({}, Encoding.fromSpec({
+      var title = axis.title(Encoding.fromSpec({
           encoding: {
             x: {name: 'a', aggregate: 'sum', axis: {titleMaxLength: '10'}}
           }
         }), 'x', layout);
-      expect(def.title).to.eql('SUM(a)');
+      expect(title).to.eql('SUM(a)');
     });
 
     it('should add return fieldTitle by default and truncate', function () {
-      var def = axis.title({}, Encoding.fromSpec({
+      var title = axis.title(Encoding.fromSpec({
           encoding: {
             x: {name: 'a', aggregate: 'sum', axis: {titleMaxLength: '3'}}
           }
         }), 'x', layout);
-      expect(def.title).to.eql('SU…');
+      expect(title).to.eql('SU…');
     });
 
 
     it('should add return fieldTitle by default and truncate', function () {
-      var def = axis.title({}, Encoding.fromSpec({
+      var title = axis.title(Encoding.fromSpec({
           encoding: {
             x: {name: 'abcdefghijkl'}
           }
         }), 'x', layout);
-      expect(def.title).to.eql('abcdefghi…');
+      expect(title).to.eql('abcdefghi…');
     });
   });