Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make offsetGridLines behavior consistent and add new offset option to scales #4545

Merged
merged 4 commits into from Aug 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/axes/cartesian/README.md
Expand Up @@ -15,6 +15,7 @@ All of the included cartesian axes support a number of common options.
| -----| ---- | --------| -----------
| `type` | `String` | | Type of scale being employed. Custom scales can be created and registered with a string key. This allows changing the type of an axis for a chart.
| `position` | `String` | | Position of the axis in the chart. Possible values are: `'top'`, `'left'`, `'bottom'`, `'right'`
| `offset` | `Boolean` | `false` | If true, extra space is added to the both edges and the axis is scaled to fit into the chart area. This is set to `true` in the bar chart by default.
| `id` | `String` | | The ID is used to link datasets and scale axes together. [more...](#axis-id)
| `gridLines` | `Object` | | Grid line configuration. [more...](../styling.md#grid-line-configuration)
| `scaleLabel` | `Object` | | Scale title configuration. [more...](../labelling.md#scale-title-configuration)
Expand Down Expand Up @@ -101,4 +102,4 @@ var myChart = new Chart(ctx, {
}
}
});
```
```
2 changes: 1 addition & 1 deletion docs/axes/styling.md
Expand Up @@ -21,7 +21,7 @@ The grid line configuration is nested under the scale configuration in the `grid
| `zeroLineColor` | Color | `'rgba(0, 0, 0, 0.25)'` | Stroke color of the grid line for the first index (index 0).
| `zeroLineBorderDash` | `Number[]` | `[]` | Length and spacing of dashes of the grid line for the first index (index 0). See [MDN](https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/setLineDash)
| `zeroLineBorderDashOffset` | `Number` | `0` | Offset for line dashes of the grid line for the first index (index 0). See [MDN](https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/lineDashOffset)
| `offsetGridLines` | `Boolean` | `false` | If true, labels are shifted to be between grid lines. This is used in the bar chart and should not generally be used.
| `offsetGridLines` | `Boolean` | `false` | If true, grid lines will be shifted to be between labels. This is set to `true` in the bar chart by default.

## Tick Configuration
The tick configuration is nested under the scale configuration in the `ticks` key. It defines options for the tick marks that are generated by the axis.
Expand Down
14 changes: 7 additions & 7 deletions docs/charts/bar.md
Expand Up @@ -93,16 +93,16 @@ The bar chart defines the following configuration options. These options are mer

| Name | Type | Default | Description
| ---- | ---- | ------- | -----------
| `barPercentage` | `Number` | `0.9` | Percent (0-1) of the available width each bar should be within the category percentage. 1.0 will take the whole category width and put the bars right next to each other. [more...](#barpercentage-vs-categorypercentage)
| `categoryPercentage` | `Number` | `0.8` | Percent (0-1) of the available width (the space between the gridlines for small datasets) for each data-point to use for the bars. [more...](#barpercentage-vs-categorypercentage)
| `barThickness` | `Number` | | Manually set width of each bar in pixels. If not set, the bars are sized automatically using `barPercentage` and `categoryPercentage`;
| `maxBarThickness` | `Number` | | Set this to ensure that the automatically sized bars are not sized thicker than this. Only works if barThickness is not set (automatic sizing is enabled).
| `gridLines.offsetGridLines` | `Boolean` | `true` | If true, the bars for a particular data point fall between the grid lines. If false, the grid line will go right down the middle of the bars. [more...](#offsetgridlines)
| `barPercentage` | `Number` | `0.9` | Percent (0-1) of the available width each bar should be within the category width. 1.0 will take the whole category width and put the bars right next to each other. [more...](#barpercentage-vs-categorypercentage)
| `categoryPercentage` | `Number` | `0.8` | Percent (0-1) of the available width each category should be within the sample width. [more...](#barpercentage-vs-categorypercentage)
| `barThickness` | `Number` | | Manually set width of each bar in pixels. If not set, the base sample widths are calculated automatically so that they take the full available widths without overlap. Then, the bars are sized using `barPercentage` and `categoryPercentage`.
| `maxBarThickness` | `Number` | | Set this to ensure that bars are not sized thicker than this.
| `gridLines.offsetGridLines` | `Boolean` | `true` | If true, the bars for a particular data point fall between the grid lines. The grid line will move to the left by one half of the tick interval. If false, the grid line will go right down the middle of the bars. [more...](#offsetgridlines)

### offsetGridLines
If true, the bars for a particular data point fall between the grid lines. If false, the grid line will go right down the middle of the bars. It is unlikely that this will ever need to be changed in practice. It exists more as a way to reuse the axis code by configuring the existing axis slightly differently.
If true, the bars for a particular data point fall between the grid lines. The grid line will move to the left by one half of the tick interval, which is the space between the grid lines. If false, the grid line will go right down the middle of the bars. This is set to true for a bar chart while false for other charts by default.

This setting applies to the axis configuration for a bar chart. If axes are added to the chart, this setting will need to be set for each new axis.
This setting applies to the axis configuration. If axes are added to the chart, this setting will need to be set for each new axis.

```javascript
options = {
Expand Down
80 changes: 56 additions & 24 deletions src/controllers/controller.bar.js
Expand Up @@ -17,6 +17,9 @@ defaults._set('bar', {
categoryPercentage: 0.8,
barPercentage: 0.9,

// offset settings
offset: true,

// grid line settings
gridLines: {
offsetGridLines: true
Expand Down Expand Up @@ -49,6 +52,9 @@ defaults._set('horizontalBar', {
categoryPercentage: 0.8,
barPercentage: 0.9,

// offset settings
offset: true,

// grid line settings
gridLines: {
offsetGridLines: true
Expand Down Expand Up @@ -234,26 +240,23 @@ module.exports = function(Chart) {
getRuler: function() {
var me = this;
var scale = me.getIndexScale();
var options = scale.options;
var stackCount = me.getStackCount();
var fullSize = scale.isHorizontal() ? scale.width : scale.height;
var tickSize = fullSize / scale.getTicks().length;
var categorySize = tickSize * options.categoryPercentage;
var fullBarSize = categorySize / stackCount;
var barSize = fullBarSize * options.barPercentage;

barSize = Math.min(
helpers.valueOrDefault(options.barThickness, barSize),
helpers.valueOrDefault(options.maxBarThickness, Infinity));
var datasetIndex = me.index;
var pixels = [];
var isHorizontal = scale.isHorizontal();
var start = isHorizontal ? scale.left : scale.top;
var end = start + (isHorizontal ? scale.width : scale.height);
var i, ilen;

for (i = 0, ilen = me.getMeta().data.length; i < ilen; ++i) {
pixels.push(scale.getPixelForValue(null, i, datasetIndex));
}

return {
pixels: pixels,
start: start,
end: end,
stackCount: stackCount,
tickSize: tickSize,
categorySize: categorySize,
categorySpacing: tickSize - categorySize,
fullBarSize: fullBarSize,
barSize: barSize,
barSpacing: fullBarSize - barSize,
scale: scale
};
},
Expand Down Expand Up @@ -308,16 +311,45 @@ module.exports = function(Chart) {
*/
calculateBarIndexPixels: function(datasetIndex, index, ruler) {
var me = this;
var scale = ruler.scale;
var isCombo = me.chart.isCombo;
var options = ruler.scale.options;
var stackIndex = me.getStackIndex(datasetIndex);
var base = scale.getPixelForValue(null, index, datasetIndex, isCombo);
var size = ruler.barSize;
var pixels = ruler.pixels;
var base = pixels[index];
var length = pixels.length;
var start = ruler.start;
var end = ruler.end;
var leftSampleSize, rightSampleSize, leftCategorySize, rightCategorySize, fullBarSize, size;

if (length === 1) {
leftSampleSize = base > start ? base - start : end - base;
rightSampleSize = base < end ? end - base : base - start;
} else {
if (index > 0) {
leftSampleSize = (base - pixels[index - 1]) / 2;
if (index === length - 1) {
rightSampleSize = leftSampleSize;
}
}
if (index < length - 1) {
rightSampleSize = (pixels[index + 1] - base) / 2;
if (index === 0) {
leftSampleSize = rightSampleSize;
}
}
}

leftCategorySize = leftSampleSize * options.categoryPercentage;
rightCategorySize = rightSampleSize * options.categoryPercentage;
fullBarSize = (leftCategorySize + rightCategorySize) / ruler.stackCount;
size = fullBarSize * options.barPercentage;

size = Math.min(
helpers.valueOrDefault(options.barThickness, size),
helpers.valueOrDefault(options.maxBarThickness, Infinity));

base -= isCombo ? ruler.tickSize / 2 : 0;
base += ruler.fullBarSize * stackIndex;
base += ruler.categorySpacing / 2;
base += ruler.barSpacing / 2;
base -= leftCategorySize;
base += fullBarSize * stackIndex;
base += (fullBarSize - size) / 2;

return {
size: size,
Expand Down
2 changes: 1 addition & 1 deletion src/controllers/controller.bubble.js
Expand Up @@ -76,7 +76,7 @@ module.exports = function(Chart) {

// Desired view properties
_model: {
x: reset ? xScale.getPixelForDecimal(0.5) : xScale.getPixelForValue(typeof data === 'object' ? data : NaN, index, dsIndex, me.chart.isCombo),
x: reset ? xScale.getPixelForDecimal(0.5) : xScale.getPixelForValue(typeof data === 'object' ? data : NaN, index, dsIndex),
y: reset ? yScale.getBasePixel() : yScale.getPixelForValue(data, index, dsIndex),
// Appearance
radius: reset ? 0 : custom.radius ? custom.radius : me.getRadius(data),
Expand Down
4 changes: 1 addition & 3 deletions src/controllers/controller.line.js
Expand Up @@ -159,8 +159,6 @@ module.exports = function(Chart) {
var xScale = me.getScaleForId(meta.xAxisID);
var pointOptions = me.chart.options.elements.point;
var x, y;
var labels = me.chart.data.labels || [];
var includeOffset = (labels.length === 1 || dataset.data.length === 1) || me.chart.isCombo;

// Compatibility: If the properties are defined with only the old name, use those values
if ((dataset.radius !== undefined) && (dataset.pointRadius === undefined)) {
Expand All @@ -170,7 +168,7 @@ module.exports = function(Chart) {
dataset.pointHitRadius = dataset.hitRadius;
}

x = xScale.getPixelForValue(typeof value === 'object' ? value : NaN, index, datasetIndex, includeOffset);
x = xScale.getPixelForValue(typeof value === 'object' ? value : NaN, index, datasetIndex);
y = reset ? yScale.getBasePixel() : me.calculatePointY(value, index, datasetIndex);

// Utility
Expand Down
9 changes: 0 additions & 9 deletions src/core/core.controller.js
Expand Up @@ -312,15 +312,6 @@ module.exports = function(Chart) {
}
}, me);

if (types.length > 1) {
for (var i = 1; i < types.length; i++) {
if (types[i] !== types[i - 1]) {
me.isCombo = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy

break;
}
}
}

return newControllers;
},

Expand Down
42 changes: 33 additions & 9 deletions src/core/core.scale.js
Expand Up @@ -8,6 +8,7 @@ var Ticks = require('./core.ticks');
defaults._set('scale', {
display: true,
position: 'left',
offset: false,

// grid line settings
gridLines: {
Expand Down Expand Up @@ -68,6 +69,19 @@ function labelsFromTicks(ticks) {
return labels;
}

function getLineValue(scale, index, offsetGridLines) {
var lineValue = scale.getPixelForTick(index);

if (offsetGridLines) {
if (index === 0) {
lineValue -= (scale.getPixelForTick(1) - lineValue) / 2;
} else {
lineValue -= (lineValue - scale.getPixelForTick(index - 1)) / 2;
}
}
return lineValue;
}

module.exports = function(Chart) {

function computeTextSize(context, tick, font) {
Expand Down Expand Up @@ -521,14 +535,15 @@ module.exports = function(Chart) {
getValueForPixel: helpers.noop,

// Used for tick location, should
getPixelForTick: function(index, includeOffset) {
getPixelForTick: function(index) {
var me = this;
var offset = me.options.offset;
if (me.isHorizontal()) {
var innerWidth = me.width - (me.paddingLeft + me.paddingRight);
var tickWidth = innerWidth / Math.max((me._ticks.length - ((me.options.gridLines.offsetGridLines) ? 0 : 1)), 1);
var tickWidth = innerWidth / Math.max((me._ticks.length - (offset ? 0 : 1)), 1);
var pixel = (tickWidth * index) + me.paddingLeft;

if (includeOffset) {
if (offset) {
pixel += tickWidth / 2;
}

Expand All @@ -541,7 +556,7 @@ module.exports = function(Chart) {
},

// Utility for getting the pixel location of a percentage of scale
getPixelForDecimal: function(decimal /* , includeOffset*/) {
getPixelForDecimal: function(decimal) {
var me = this;
if (me.isHorizontal()) {
var innerWidth = me.width - (me.paddingLeft + me.paddingRight);
Expand Down Expand Up @@ -659,7 +674,7 @@ module.exports = function(Chart) {
helpers.each(ticks, function(tick, index) {
var label = tick.label;
var lineWidth, lineColor, borderDash, borderDashOffset;
if (index === (typeof me.zeroLineIndex !== 'undefined' ? me.zeroLineIndex : 0)) {
if (index === (typeof me.zeroLineIndex !== 'undefined' ? me.zeroLineIndex : 0) && (options.offset === gridLines.offsetGridLines)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] what's the reason for the extra condition here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents the zero line from being drawn in case of 2. and 3. below.
screen shot 2017-08-10 at 11 37 10 am

// Draw the first index specially
lineWidth = gridLines.zeroLineWidth;
lineColor = gridLines.zeroLineColor;
Expand Down Expand Up @@ -693,8 +708,13 @@ module.exports = function(Chart) {
labelY = me.bottom - labelYOffset;
}

var xLineValue = me.getPixelForTick(index) + helpers.aliasPixel(lineWidth); // xvalues for grid lines
labelX = me.getPixelForTick(index, gridLines.offsetGridLines) + optionTicks.labelOffset; // x values for optionTicks (need to consider offsetLabel option)
var xLineValue = getLineValue(me, index, gridLines.offsetGridLines && ticks.length > 1);
if (xLineValue < me.left) {
lineColor = 'rgba(0,0,0,0)';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to "hide" the grid line here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents the leftmost grid line from being drawn outsize the chart area like this.

screen shot 2017-08-12 at 3 35 45 am

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to skip that line instead? if (xLineValue < me.left) { return; }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since a grid line and a label are drawn together in the function, if we skip the line, the label will be also skipped ('1' in the chart above).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right

}
xLineValue += helpers.aliasPixel(lineWidth);

labelX = me.getPixelForTick(index) + optionTicks.labelOffset; // x values for optionTicks (need to consider offsetLabel option)

tx1 = tx2 = x1 = x2 = xLineValue;
ty1 = yTickStart;
Expand All @@ -715,9 +735,13 @@ module.exports = function(Chart) {

labelX = isLeft ? me.right - labelXOffset : me.left + labelXOffset;

var yLineValue = me.getPixelForTick(index); // xvalues for grid lines
var yLineValue = getLineValue(me, index, gridLines.offsetGridLines && ticks.length > 1);
if (yLineValue < me.top) {
lineColor = 'rgba(0,0,0,0)';
}
yLineValue += helpers.aliasPixel(lineWidth);
labelY = me.getPixelForTick(index, gridLines.offsetGridLines) + optionTicks.labelOffset;

labelY = me.getPixelForTick(index) + optionTicks.labelOffset;

tx1 = xTickStart;
tx2 = xTickEnd;
Expand Down
20 changes: 11 additions & 9 deletions src/scales/scale.category.js
Expand Up @@ -60,10 +60,11 @@ module.exports = function(Chart) {
},

// Used to get data value locations. Value can either be an index or a numerical value
getPixelForValue: function(value, index, datasetIndex, includeOffset) {
getPixelForValue: function(value, index) {
var me = this;
var offset = me.options.offset;
// 1 is added because we need the length but we have the indexes
var offsetAmt = Math.max((me.maxIndex + 1 - me.minIndex - ((me.options.gridLines.offsetGridLines) ? 0 : 1)), 1);
var offsetAmt = Math.max((me.maxIndex + 1 - me.minIndex - (offset ? 0 : 1)), 1);

// If value is a data object, then index is the index in the data array,
// not the index of the scale. We need to change that.
Expand All @@ -82,7 +83,7 @@ module.exports = function(Chart) {
var valueWidth = me.width / offsetAmt;
var widthOffset = (valueWidth * (index - me.minIndex));

if (me.options.gridLines.offsetGridLines && includeOffset || me.maxIndex === me.minIndex && includeOffset) {
if (offset) {
widthOffset += (valueWidth / 2);
}

Expand All @@ -91,25 +92,26 @@ module.exports = function(Chart) {
var valueHeight = me.height / offsetAmt;
var heightOffset = (valueHeight * (index - me.minIndex));

if (me.options.gridLines.offsetGridLines && includeOffset) {
if (offset) {
heightOffset += (valueHeight / 2);
}

return me.top + Math.round(heightOffset);
},
getPixelForTick: function(index, includeOffset) {
return this.getPixelForValue(this.ticks[index], index + this.minIndex, null, includeOffset);
getPixelForTick: function(index) {
return this.getPixelForValue(this.ticks[index], index + this.minIndex, null);
},
getValueForPixel: function(pixel) {
var me = this;
var offset = me.options.offset;
var value;
var offsetAmt = Math.max((me.ticks.length - ((me.options.gridLines.offsetGridLines) ? 0 : 1)), 1);
var offsetAmt = Math.max((me._ticks.length - (offset ? 0 : 1)), 1);
var horz = me.isHorizontal();
var valueDimension = (horz ? me.width : me.height) / offsetAmt;

pixel -= horz ? me.left : me.top;

if (me.options.gridLines.offsetGridLines) {
if (offset) {
pixel -= (valueDimension / 2);
}

Expand All @@ -119,7 +121,7 @@ module.exports = function(Chart) {
value = Math.round(pixel / valueDimension);
}

return value;
return value + me.minIndex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it is fixing a pre-existing bug. Do you know if there is a ticket for it? If so, we should add a test to cover this case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a ticket for it. getValueForPixel() is only called from test code.

},
getBasePixel: function() {
return this.bottom;
Expand Down