-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ var Ticks = require('./core.ticks'); | |
defaults._set('scale', { | ||
display: true, | ||
position: 'left', | ||
offset: false, | ||
|
||
// grid line settings | ||
gridLines: { | ||
|
@@ -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) { | ||
|
@@ -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; | ||
} | ||
|
||
|
@@ -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); | ||
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [question] what's the reason for the extra condition here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// Draw the first index specially | ||
lineWidth = gridLines.zeroLineWidth; | ||
lineColor = gridLines.zeroLineColor; | ||
|
@@ -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)'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to "hide" the grid line here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to skip that line instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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); | ||
} | ||
|
||
|
@@ -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); | ||
} | ||
|
||
|
@@ -119,7 +121,7 @@ module.exports = function(Chart) { | |
value = Math.round(pixel / valueDimension); | ||
} | ||
|
||
return value; | ||
return value + me.minIndex; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there is a ticket for it. |
||
}, | ||
getBasePixel: function() { | ||
return this.bottom; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.