Skip to content

Commit

Permalink
Revert "Scale: draw offset grid for labels before autoSkip (#8748)" (#…
Browse files Browse the repository at this point in the history
…8752)

This reverts commit cdba66c.
  • Loading branch information
kurkle authored Mar 30, 2021
1 parent cdba66c commit 1a5a152
Show file tree
Hide file tree
Showing 21 changed files with 33 additions and 99 deletions.
2 changes: 1 addition & 1 deletion docs/docs/axes/styling.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Namespace: `options.scales[scaleId].grid`, it defines options for the grid lines
| `drawOnChartArea` | `boolean` | | | `true` | If true, draw lines on the chart area inside the axis lines. This is useful when there are multiple axes and you need to control which grid lines are drawn.
| `drawTicks` | `boolean` | | | `true` | If true, draw lines beside the ticks in the axis area beside the chart.
| `lineWidth` | `number` | Yes | Yes | `1` | Stroke width of grid lines.
| `offset` | `boolean` | | | `false` | If true, grid lines will be shifted to be between labels. This is set to `true` for a bar chart by default. Note: AutoSkip does not remove offset grid lines.
| `offset` | `boolean` | | | `false` | If true, grid lines will be shifted to be between labels. This is set to `true` for a bar chart by default.
| `tickBorderDash` | `number[]` | | | | Length and spacing of the tick mark line. If not set, defaults to the grid line `borderDash` value.
| `tickBorderDashOffset` | `number` | Yes | Yes | | Offset for the line dash of the tick mark. If unset, defaults to the grid line `borderDashOffset` value
| `tickColor` | [`Color`](../general/colors.md) | Yes | Yes | | Color of the tick line. If unset, defaults to the grid line color.
Expand Down
24 changes: 11 additions & 13 deletions src/core/core.scale.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,21 @@ function sample(arr, numItems) {
* @param {boolean} offsetGridLines
*/
function getPixelForGridLine(scale, index, offsetGridLines) {
const length = offsetGridLines ? scale._allTicks.length : scale.ticks.length;
const length = scale.ticks.length;
const validIndex = Math.min(index, length - 1);
const start = scale._startPixel;
const end = scale._endPixel;
const epsilon = 1e-6; // 1e-6 is margin in pixels for accumulated error.
let lineValue = scale.getPixelForTick(validIndex, offsetGridLines);
let lineValue = scale.getPixelForTick(validIndex);
let offset;

if (offsetGridLines) {
if (length === 1) {
offset = Math.max(lineValue - start, end - lineValue);
} else if (index === 0) {
offset = (scale.getPixelForTick(1, offsetGridLines) - lineValue) / 2;
offset = (scale.getPixelForTick(1) - lineValue) / 2;
} else {
offset = (lineValue - scale.getPixelForTick(validIndex - 1, offsetGridLines)) / 2;
offset = (lineValue - scale.getPixelForTick(validIndex - 1)) / 2;
}
lineValue += validIndex < index ? offset : -offset;

Expand Down Expand Up @@ -205,7 +205,7 @@ export default class Scale extends Element {
this.min = undefined;
this.max = undefined;
/** @type {Tick[]} */
this.ticks = this._allTicks = [];
this.ticks = [];
/** @type {object[]|null} */
this._gridLineItems = null;
/** @type {object[]|null} */
Expand Down Expand Up @@ -428,7 +428,6 @@ export default class Scale extends Element {
me.afterCalculateLabelRotation();

// Auto-skip
me._allTicks = me.ticks;
if (tickOpts.display && (tickOpts.autoSkip || tickOpts.source === 'auto')) {
me.ticks = autoSkip(me, me.ticks);
me._labelSizes = null;
Expand Down Expand Up @@ -667,7 +666,7 @@ export default class Scale extends Element {

_calculatePadding(first, last, sin, cos) {
const me = this;
const {position, ticks: {align, padding}} = me.options;
const {ticks: {align, padding}, position} = me.options;
const isRotated = me.labelRotation !== 0;
const labelsBelowTicks = position !== 'top' && me.axis === 'x';

Expand Down Expand Up @@ -697,8 +696,8 @@ export default class Scale extends Element {
}

// Adjust padding taking into account changes in offsets
me.paddingLeft = Math.max((paddingLeft - offsetLeft + padding) * me.width / (me.width - offsetLeft), padding);
me.paddingRight = Math.max((paddingRight - offsetRight + padding) * me.width / (me.width - offsetRight), padding);
me.paddingLeft = Math.max((paddingLeft - offsetLeft + padding) * me.width / (me.width - offsetLeft), 0);
me.paddingRight = Math.max((paddingRight - offsetRight + padding) * me.width / (me.width - offsetRight), 0);
} else {
let paddingTop = last.height / 2;
let paddingBottom = first.height / 2;
Expand Down Expand Up @@ -872,11 +871,10 @@ export default class Scale extends Element {
* Returns the location of the tick at the given index
* The coordinate (0, 0) is at the upper-left corner of the canvas
* @param {number} index
* @param {boolean} [all] - use ticks before autoSkip
* @return {number}
*/
getPixelForTick(index, all = false) {
const ticks = all ? this._allTicks : this.ticks;
getPixelForTick(index) {
const ticks = this.ticks;
if (index < 0 || index > ticks.length - 1) {
return null;
}
Expand Down Expand Up @@ -994,7 +992,7 @@ export default class Scale extends Element {
const {grid, position} = options;
const offset = grid.offset;
const isHorizontal = me.isHorizontal();
const ticks = offset ? me._allTicks : me.ticks;
const ticks = me.ticks;
const ticksLength = ticks.length + (offset ? 1 : 0);
const tl = getTickMarkLength(grid);
const items = [];
Expand Down
11 changes: 11 additions & 0 deletions src/scales/scale.category.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,17 @@ export default class CategoryScale extends Scale {
return value === null ? NaN : me.getPixelForDecimal((value - me._startValue) / me._valueRange);
}

// Must override base implementation because it calls getPixelForValue
// and category scale can have duplicate values
getPixelForTick(index) {
const me = this;
const ticks = me.ticks;
if (index < 0 || index > ticks.length - 1) {
return null;
}
return me.getPixelForValue(ticks[index].value);
}

getValueForPixel(pixel) {
const me = this;
return Math.round(me._startValue + me.getDecimalForPixel(pixel) * me._valueRange);
Expand Down
Binary file modified test/fixtures/core.scale/autoSkip/offset.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/core.scale/crossAlignment/cross-align-bottom-far.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/core.scale/crossAlignment/cross-align-top-center.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/core.scale/crossAlignment/cross-align-top-far.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/core.scale/crossAlignment/cross-align-top-near.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
38 changes: 0 additions & 38 deletions test/fixtures/scale.category/autoskip-grid-x.js

This file was deleted.

Binary file removed test/fixtures/scale.category/autoskip-grid-x.png
Binary file not shown.
37 changes: 0 additions & 37 deletions test/fixtures/scale.category/autoskip-grid-y.js

This file was deleted.

Binary file removed test/fixtures/scale.category/autoskip-grid-y.png
Binary file not shown.
Binary file modified test/fixtures/scale.time/offset-with-1-tick.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/scale.time/offset-with-2-ticks.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/scale.timeseries/financial-daily.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 6 additions & 6 deletions test/specs/core.layouts.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ describe('Chart.layouts', function() {

expect(chart.chartArea.bottom).toBeCloseToPixel(120);
expect(chart.chartArea.left).toBeCloseToPixel(31);
expect(chart.chartArea.right).toBeCloseToPixel(247);
expect(chart.chartArea.right).toBeCloseToPixel(250);
expect(chart.chartArea.top).toBeCloseToPixel(32);

// Is xScale at the right spot
expect(chart.scales.x.bottom).toBeCloseToPixel(150);
expect(chart.scales.x.left).toBeCloseToPixel(31);
expect(chart.scales.x.right).toBeCloseToPixel(247);
expect(chart.scales.x.right).toBeCloseToPixel(250);
expect(chart.scales.x.top).toBeCloseToPixel(120);
expect(chart.scales.x.labelRotation).toBeCloseTo(0);

Expand Down Expand Up @@ -79,13 +79,13 @@ describe('Chart.layouts', function() {
});

expect(chart.chartArea.bottom).toBeCloseToPixel(139);
expect(chart.chartArea.left).toBeCloseToPixel(3);
expect(chart.chartArea.left).toBeCloseToPixel(0);
expect(chart.chartArea.right).toBeCloseToPixel(218);
expect(chart.chartArea.top).toBeCloseToPixel(62);

// Is xScale at the right spot
expect(chart.scales.x.bottom).toBeCloseToPixel(62);
expect(chart.scales.x.left).toBeCloseToPixel(3);
expect(chart.scales.x.left).toBeCloseToPixel(0);
expect(chart.scales.x.right).toBeCloseToPixel(218);
expect(chart.scales.x.top).toBeCloseToPixel(32);
expect(chart.scales.x.labelRotation).toBeCloseTo(0);
Expand Down Expand Up @@ -160,13 +160,13 @@ describe('Chart.layouts', function() {

expect(chart.chartArea.bottom).toBeCloseToPixel(110);
expect(chart.chartArea.left).toBeCloseToPixel(70);
expect(chart.chartArea.right).toBeCloseToPixel(247);
expect(chart.chartArea.right).toBeCloseToPixel(250);
expect(chart.chartArea.top).toBeCloseToPixel(32);

// Is xScale at the right spot
expect(chart.scales.x.bottom).toBeCloseToPixel(150);
expect(chart.scales.x.left).toBeCloseToPixel(70);
expect(chart.scales.x.right).toBeCloseToPixel(247);
expect(chart.scales.x.right).toBeCloseToPixel(250);
expect(chart.scales.x.top).toBeCloseToPixel(110);
expect(chart.scales.x.labelRotation).toBeCloseTo(40, -1);

Expand Down
4 changes: 2 additions & 2 deletions test/specs/platform.basic.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ describe('Platform.basic', function() {

expect(chart.chartArea.bottom).toBeCloseToPixel(120);
expect(chart.chartArea.left).toBeCloseToPixel(31);
expect(chart.chartArea.right).toBeCloseToPixel(247);
expect(chart.chartArea.right).toBeCloseToPixel(250);
expect(chart.chartArea.top).toBeCloseToPixel(32);
});

Expand Down Expand Up @@ -84,7 +84,7 @@ describe('Platform.basic', function() {

expect(chart.chartArea.bottom).toBeCloseToPixel(150);
expect(chart.chartArea.left).toBeCloseToPixel(31);
expect(chart.chartArea.right).toBeCloseToPixel(297);
expect(chart.chartArea.right).toBeCloseToPixel(300);
expect(chart.chartArea.top).toBeCloseToPixel(32);
});
});
Expand Down
2 changes: 1 addition & 1 deletion test/specs/scale.category.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ describe('Category scale tests', function() {
var xScale = chart.scales.x;
expect(xScale.getPixelForValue(0)).toBeCloseToPixel(89);
expect(xScale.getPixelForValue(3)).toBeCloseToPixel(451);
expect(xScale.getPixelForValue(4)).toBeCloseToPixel(570);
expect(xScale.getPixelForValue(4)).toBeCloseToPixel(572);
});

it('Should get the correct pixel for an object value in a horizontal bar chart', function() {
Expand Down
2 changes: 1 addition & 1 deletion test/specs/scale.time.tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -1114,7 +1114,7 @@ describe('Time scale tests', function() {
});
const scale = chart.scales.x;
expect(scale.getPixelForDecimal(0)).toBeCloseToPixel(29);
expect(scale.getPixelForDecimal(1.0)).toBeCloseToPixel(509);
expect(scale.getPixelForDecimal(1.0)).toBeCloseToPixel(512);
});

['data', 'labels'].forEach(function(source) {
Expand Down

0 comments on commit 1a5a152

Please sign in to comment.