Skip to content

Commit

Permalink
Fix handling of "original" with panning nonlinear scales
Browse files Browse the repository at this point in the history
Nonlinear scales failed to resolve "original" within panNumericalScale.

To fix this:

* Resolving "original" within panNumericalScale seemed redundant, when
  updateRange was already doing it, so I instead moved the logic into
  updateRange and control it with a new special value for the `zoom`
  parameter.  (Hopefully the use of a special string like this is
  acceptable as long as JSDoc calls it out - if I should use another
  approach, please let me know.)
* This change put updateRange over the configured ESLint complexity
  limit, so I extracted a new getScaleLimits function to reduce
  complexity.

Add unit tests, both for this bug and for the preexisting but untested
"original" feature.
  • Loading branch information
joshkel committed Jul 25, 2023
1 parent 423f7ee commit 1ce98cb
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 10 deletions.
33 changes: 23 additions & 10 deletions src/scale.types.js
Expand Up @@ -6,6 +6,7 @@ import {getState} from './state';
* @typedef {import('chart.js').Scale} Scale
* @typedef {import('../types/options').LimitOptions} LimitOptions
* @typedef {{min: number, max: number}} ScaleRange
* @typedef {import('../types/options').ScaleLimits} ScaleLimits
*/

/**
Expand Down Expand Up @@ -33,6 +34,15 @@ function zoomDelta(scale, zoom, center) {
};
}

/**
* @param {Scale} scale
* @param {LimitOptions|undefined} limits
* @returns {ScaleLimits}
*/
function getScaleLimits(scale, limits) {
return limits && (limits[scale.id] || limits[scale.axis]) || {};
}

function getLimit(state, scale, scaleLimits, prop, fallback) {
let limit = scaleLimits[prop];
if (limit === 'original') {
Expand All @@ -59,20 +69,25 @@ function getRange(scale, pixel0, pixel1) {

/**
* @param {Scale} scale
* @param {ScaleRange} limits
* @param {LimitOptions} limits
* @param {boolean} zoom
* @param {ScaleRange} minMax
* @param {LimitOptions} [limits]
* @param {boolean|'pan'} [zoom]
* @returns {boolean}
*/
export function updateRange(scale, {min, max}, limits, zoom = false) {
const state = getState(scale.chart);
const {id, axis, options: scaleOpts} = scale;
const {options: scaleOpts} = scale;

const scaleLimits = limits && (limits[id] || limits[axis]) || {};
const scaleLimits = getScaleLimits(scale, limits);
const {minRange = 0} = scaleLimits;
const minLimit = getLimit(state, scale, scaleLimits, 'min', -Infinity);
const maxLimit = getLimit(state, scale, scaleLimits, 'max', Infinity);

if (zoom === 'pan' && (min < minLimit || max > maxLimit)) {
// At limit: No change but return true to indicate no need to store the delta.
return true;
}

const range = zoom ? Math.max(max - min, minRange) : scale.max - scale.min;
const offset = (range - max + min) / 2;
min -= offset;
Expand Down Expand Up @@ -165,20 +180,18 @@ const OFFSETS = {
year: 182 * 24 * 60 * 60 * 1000 // 182 d
};

function panNumericalScale(scale, delta, limits, canZoom = false) {
function panNumericalScale(scale, delta, limits, pan = false) {
const {min: prevStart, max: prevEnd, options} = scale;
const round = options.time && options.time.round;
const offset = OFFSETS[round] || 0;
const newMin = scale.getValueForPixel(scale.getPixelForValue(prevStart + offset) - delta);
const newMax = scale.getValueForPixel(scale.getPixelForValue(prevEnd + offset) - delta);
const {min: minLimit = -Infinity, max: maxLimit = Infinity} = canZoom && limits && limits[scale.axis] || {};
if (isNaN(newMin) || isNaN(newMax) || newMin < minLimit || newMax > maxLimit) {
// At limit: No change but return true to indicate no need to store the delta.
if (isNaN(newMin) || isNaN(newMax)) {
// NaN can happen for 0-dimension scales (either because they were configured
// with min === max or because the chart has 0 plottable area).
return true;
}
return updateRange(scale, {min: newMin, max: newMax}, limits, canZoom);
return updateRange(scale, {min: newMin, max: newMax}, limits, pan ? 'pan' : false);
}

function panNonLinearScale(scale, delta, limits) {
Expand Down
71 changes: 71 additions & 0 deletions test/specs/pan.spec.js
Expand Up @@ -124,6 +124,77 @@ describe('pan', function() {
expect(scale.options.min).toBe(2);
expect(scale.options.max).toBe(2);
});

it('should respect original limits', function() {
const chart = window.acquireChart({
type: 'line',
data,
options: {
plugins: {
zoom: {
pan: {
enabled: true,
mode: 'x',
},
limits: {
x: {
min: 'original',
max: 'original',
}
},
}
},
scales: {
x: {
min: 1,
max: 2
}
}
}
});
const scale = chart.scales.x;
expect(scale.min).toBe(1);
expect(scale.max).toBe(2);
chart.pan(100);
expect(scale.min).toBe(1);
expect(scale.max).toBe(2);
});

it('should respect original limits for nonlinear scales', function() {
const chart = window.acquireChart({
type: 'line',
data,
options: {
plugins: {
zoom: {
pan: {
enabled: true,
mode: 'x',
},
limits: {
x: {
min: 'original',
max: 'original',
}
},
}
},
scales: {
x: {
type: 'logarithmic',
min: 1,
max: 10
}
}
}
});
const scale = chart.scales.x;
expect(scale.min).toBe(1);
expect(scale.max).toBe(10);
chart.pan(100);
expect(scale.min).toBe(1);
expect(scale.max).toBe(10);
});
});

describe('events', function() {
Expand Down

0 comments on commit 1ce98cb

Please sign in to comment.